{"thread":{"id":"55018","subject":"[PATCH 1/9] cache-tree: clean up cache_tree_update()","startedAt":"2021-01-20T16:55:12Z","lastAt":"2021-01-26T20:52:19Z","messageCount":65,"participants":["Derrick Stolee via GitGitGadget","Elijah Newren","Derrick Stolee","Chris Torek","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"414812","messageId":"0bccfd34ae5924aef0432fd6727debb75c052da5.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 1/9] cache-tree: clean up cache_tree_update()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:51Z","receivedAt":"2021-01-20T16:55:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMake the method safer by allocating a cache_tree member for the given\nindex_state if it is not already present.\n\nAlso drop local variables that are used exactly once and can be found\ndirectly from the 'istate' parameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 3f1a8d4f1b7..c1e49901c17 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n-\tstruct cache_tree *it = istate->cache_tree;\n-\tstruct cache_entry **cache = istate->cache;\n-\tint entries = istate->cache_nr;\n-\tint skip, i = verify_cache(cache, entries, flags);\n+\tint skip, i;\n+\n+\ti = verify_cache(istate->cache, istate->cache_nr, flags);\n \n \tif (i)\n \t\treturn i;\n+\n+\tif (!istate->cache_tree)\n+\t\tistate->cache_tree = cache_tree();\n+\n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\ti = update_one(it, cache, entries, \"\", 0, &skip, flags);\n+\ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n+\t\t       \"\", 0, &skip, flags);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n-- \ngitgitgadget\n\n"},{"id":"414813","messageId":"838922de2e9756c00f4a159b2b8722ae4d28b011.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 3/9] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:53Z","receivedAt":"2021-01-20T16:55:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe index has an fsmonitor_dirty bitmap that records which index entries\nare \"dirty\" based on the response from the FSMonitor. If this bitmap\never grows larger than the index, then there was an error in how it was\nconstructed, and it was probably a developer's bug.\n\nThere are several BUG() statements that are very similar, so replace\nthese uses with a simpler assert_index_minimum(). Since there is one\ncaller that uses a custom 'pos' value instead of the bit_size member, we\ncannot simplify it too much. However, the error string is identical in\neach, so this simplifies things.\n\nThe end result is that the code is simpler to read while also preserving\nthese assertions for developers in the FSMonitor space.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n fsmonitor.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ca031c3abb8..52a50a9545a 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -13,14 +13,19 @@\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n+static void assert_index_minimum(struct index_state *istate, size_t pos)\n+{\n+\tif (pos > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n+\t\t    (uintmax_t)pos, istate->cache_nr);\n+}\n+\n static void fsmonitor_ewah_callback(size_t pos, void *is)\n {\n \tstruct index_state *istate = (struct index_state *)is;\n \tstruct cache_entry *ce;\n \n-\tif (pos >= istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n-\t\t    (uintmax_t)pos, istate->cache_nr);\n+\tassert_index_minimum(istate, pos);\n \n \tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n@@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t}\n \n \t\t\t/* Mark all previously saved entries as dirty */\n-\t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\t\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n \t\t\trefresh_fsmonitor(istate);\n-- \ngitgitgadget\n\n"},{"id":"414814","messageId":"d4ff0468fc0e22fe3ef82f74603ac781fd7711e1.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 4/9] repository: add repo reference to index_state","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:54Z","receivedAt":"2021-01-20T16:56:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nIt will be helpful to add behavior to index opertations that might\ntrigger an object lookup. Since each index belongs to a specific\nrepository, add a 'repo' pointer to struct index_state that allows\naccess to this repository.\n\nThis will prevent future changes from needing to pass an additional\n'struct repository *repo' parameter and instead rely only on the 'struct\nindex_state *istate' parameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache.h      | 1 +\n repository.c | 4 ++++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 71097657489..f9c7a603841 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -328,6 +328,7 @@ struct index_state {\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\n+\tstruct repository *repo;\n };\n \n /* Name hashing */\ndiff --git a/repository.c b/repository.c\nindex a4174ddb062..67a4c1da2d9 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)\n \tif (!repo->index)\n \t\trepo->index = xcalloc(1, sizeof(*repo->index));\n \n+\t/* Complete the double-reference */\n+\tif (!repo->index->repo)\n+\t\trepo->index->repo = repo;\n+\n \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"414815","messageId":"3ba4b35f09cf3dc313787cdec6856286fe97d8a8.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 5/9] name-hash: use trace2 regions for init","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:55Z","receivedAt":"2021-01-20T16:56:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe lazy_init_name_hash() populates a hashset with all filenames and\nanother with all directories represented in the index. This is run only\nif we need to use the hashsets to check for existence or case-folding\nrenames.\n\nPlace trace2 regions where there is already a performance trace.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n name-hash.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex 5d3c7b12c18..4e03fac9bb1 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -7,6 +7,7 @@\n  */\n #include \"cache.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct dir_entry {\n \tstruct hashmap_entry ent;\n@@ -577,6 +578,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \tif (istate->name_hash_initialized)\n \t\treturn;\n \ttrace_performance_enter();\n+\ttrace2_region_enter(\"index\", \"name-hash-init\", istate->repo);\n \thashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n \thashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);\n \n@@ -597,6 +599,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t}\n \n \tistate->name_hash_initialized = 1;\n+\ttrace2_region_leave(\"index\", \"name-hash-init\", istate->repo);\n \ttrace_performance_leave(\"initialize name hash\");\n }\n \n-- \ngitgitgadget\n\n"},{"id":"414816","messageId":"64358ec7ea2b3df4a8f1099452579a7568996921.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 6/9] sparse-checkout: load sparse-checkout patterns","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:56Z","receivedAt":"2021-01-20T16:56:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nA future feature will want to load the sparse-checkout patterns into a\npattern_list, but the current mechanism to do so is a bit complicated.\nThis is made difficult due to needing to find the sparse-checkout file\nin different ways throughout the codebase.\n\nThe logic implemented in the new get_sparse_checkout_patterns() was\nduplicated in populate_from_existing_patterns() in unpack-trees.c. Use\nthe new method instead, keeping the logic around handling the struct\nunpack_trees_options.\n\nThe callers to get_sparse_checkout_filename() in\nbuiltin/sparse-checkout.c manipulate the sparse-checkout file directly,\nso it is not appropriate to replace logic in that file with\nget_sparse_checkout_patterns().\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/sparse-checkout.c |  5 -----\n dir.c                     | 17 +++++++++++++++++\n dir.h                     |  2 ++\n unpack-trees.c            |  6 +-----\n 4 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex e3140db2a0a..2306a9ad98e 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {\n \tNULL\n };\n \n-static char *get_sparse_checkout_filename(void)\n-{\n-\treturn git_pathdup(\"info/sparse-checkout\");\n-}\n-\n static void write_patterns_to_file(FILE *fp, struct pattern_list *pl)\n {\n \tint i;\ndiff --git a/dir.c b/dir.c\nindex d637461da5c..d153a63bbd1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2998,6 +2998,23 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t}\n }\n \n+char *get_sparse_checkout_filename(void)\n+{\n+\treturn git_pathdup(\"info/sparse-checkout\");\n+}\n+\n+int get_sparse_checkout_patterns(struct pattern_list *pl)\n+{\n+\tint res;\n+\tchar *sparse_filename = get_sparse_checkout_filename();\n+\n+\tpl->use_cone_patterns = core_sparse_checkout_cone;\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL);\n+\n+\tfree(sparse_filename);\n+\treturn res;\n+}\n+\n int remove_path(const char *name)\n {\n \tchar *slash;\ndiff --git a/dir.h b/dir.h\nindex a3c40dec516..facfae47402 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -448,6 +448,8 @@ int is_empty_dir(const char *dir);\n \n void setup_standard_excludes(struct dir_struct *dir);\n \n+char *get_sparse_checkout_filename(void);\n+int get_sparse_checkout_patterns(struct pattern_list *pl);\n \n /* Constants for remove_dir_recursively: */\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex af6e9b9c2fd..837b8bb42fb 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1549,14 +1549,10 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n static void populate_from_existing_patterns(struct unpack_trees_options *o,\n \t\t\t\t\t    struct pattern_list *pl)\n {\n-\tchar *sparse = git_pathdup(\"info/sparse-checkout\");\n-\n-\tpl->use_cone_patterns = core_sparse_checkout_cone;\n-\tif (add_patterns_from_file_to_list(sparse, \"\", 0, pl, NULL) < 0)\n+\tif (get_sparse_checkout_patterns(pl) < 0)\n \t\to->skip_sparse_checkout = 1;\n \telse\n \t\to->pl = pl;\n-\tfree(sparse);\n }\n \n \n-- \ngitgitgadget\n\n"},{"id":"414817","messageId":"91344f5108c835a48b2460b9de75c489045b9cce.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 7/9] sparse-checkout: hold pattern list in index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:57Z","receivedAt":"2021-01-20T16:57:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nAs we modify the sparse-checkout definition, we perform index operations\non a pattern_list that only exists in-memory. This allows easy backing\nout in case the index update fails.\n\nHowever, if the index write itself cares about the sparse-checkout\npattern set, we need access to that in-memory copy. Place a pointer to\na 'struct pattern_list' in the index so we can access this on-demand.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/sparse-checkout.c | 17 ++++++++++-------\n cache.h                   |  2 ++\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 2306a9ad98e..e00b82af727 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)\n \tif (is_index_unborn(r->index))\n \t\treturn UPDATE_SPARSITY_SUCCESS;\n \n+\tr->index->sparse_checkout_patterns = pl;\n+\n \tmemset(&o, 0, sizeof(o));\n \to.verbose_update = isatty(2);\n \to.update = 1;\n@@ -138,6 +140,7 @@ static int update_working_directory(struct pattern_list *pl)\n \telse\n \t\trollback_lock_file(&lock_file);\n \n+\tr->index->sparse_checkout_patterns = NULL;\n \treturn result;\n }\n \n@@ -517,19 +520,18 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)\n {\n \tint result;\n \tint changed_config = 0;\n-\tstruct pattern_list pl;\n-\tmemset(&pl, 0, sizeof(pl));\n+\tstruct pattern_list *pl = xcalloc(1, sizeof(*pl));\n \n \tswitch (m) {\n \tcase ADD:\n \t\tif (core_sparse_checkout_cone)\n-\t\t\tadd_patterns_cone_mode(argc, argv, &pl);\n+\t\t\tadd_patterns_cone_mode(argc, argv, pl);\n \t\telse\n-\t\t\tadd_patterns_literal(argc, argv, &pl);\n+\t\t\tadd_patterns_literal(argc, argv, pl);\n \t\tbreak;\n \n \tcase REPLACE:\n-\t\tadd_patterns_from_input(&pl, argc, argv);\n+\t\tadd_patterns_from_input(pl, argc, argv);\n \t\tbreak;\n \t}\n \n@@ -539,12 +541,13 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)\n \t\tchanged_config = 1;\n \t}\n \n-\tresult = write_patterns_and_update(&pl);\n+\tresult = write_patterns_and_update(pl);\n \n \tif (result && changed_config)\n \t\tset_config(MODE_NO_PATTERNS);\n \n-\tclear_pattern_list(&pl);\n+\tclear_pattern_list(pl);\n+\tfree(pl);\n \treturn result;\n }\n \ndiff --git a/cache.h b/cache.h\nindex f9c7a603841..bf4ec3de4b0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -305,6 +305,7 @@ static inline unsigned int canon_mode(unsigned int mode)\n struct split_index;\n struct untracked_cache;\n struct progress;\n+struct pattern_list;\n \n struct index_state {\n \tstruct cache_entry **cache;\n@@ -329,6 +330,7 @@ struct index_state {\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\n \tstruct repository *repo;\n+\tstruct pattern_list *sparse_checkout_patterns;\n };\n \n /* Name hashing */\n-- \ngitgitgadget\n\n"},{"id":"414818","messageId":"8326a9b5320e1e774caef568fcce2bfd2ec13cb1.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 8/9] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:58Z","receivedAt":"2021-01-20T16:57:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMost test cases can verify Git's behavior using input/output\nexpectations or changes to the .git directory. However, sometimes we\nwant to check that Git did or did not run a certain section of code.\nThis is particularly important for performance-only features that we\nwant to ensure have been enabled in certain cases.\n\nAdd a new 'test_region' function that checks if a trace2 region was\nentered and left in a given trace2 event log.\n\nThere is one existing test (t0500-progress-display.sh) that performs\nthis check already, so use the helper function instead. More uses will\nbe added in a later change.\n\nt6423-merge-rename-directories.sh also greps for region_enter lines, but\nit verifies the number of such lines, which is not the same as an\nexistence check.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t0500-progress-display.sh |  3 +--\n t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 1ed1df351cb..c461b89dfaf 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '\n \t\t\"Working hard\" <in 2>stderr &&\n \n \t# t0212/parse_events.perl intentionally omits regions and data.\n-\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n-\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\ttest_region category progress trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a9..c878db93013 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1655,3 +1655,43 @@ test_subcommand () {\n \t\tgrep \"\\[$expr\\]\"\n \tfi\n }\n+\n+# Check that the given command was invoked as part of the\n+# trace2-format trace on stdin.\n+#\n+#\ttest_region [!] <category> <label> git <command> <args>...\n+#\n+# For example, to look for trace2_region_enter(\"index\", \"do_read_index\", repo)\n+# in an invocation of \"git checkout HEAD~1\", run\n+#\n+#\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+#\t\tgit checkout HEAD~1 &&\n+#\ttest_region index do_read_index <trace.txt\n+#\n+# If the first parameter passed is !, this instead checks that\n+# the given region was not entered.\n+#\n+test_region () {\n+\tlocal expect_exit=0\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\texpect_exit=1\n+\t\tshift\n+\tfi\n+\n+\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+}\n-- \ngitgitgadget\n\n"},{"id":"414819","messageId":"pull.839.git.1611161639.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":null,"subject":"[PATCH 0/9] More index cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:50Z","receivedAt":"2021-01-20T16:57:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is based on ds/cache-tree-basics.\n\nHere are a few more cleanups that are vaguely related to the index. I\ndiscovered these while preparing my sparse-index RFC that I intend to send\nearly next week.\n\nThe biggest patch is the final one, which creates a test script for\ncomparing sparse-checkouts to full checkouts. There are some commands that\ndo not behave similarly. This script will be the backbone of my testing\nstrategy for the sparse-index by adding a new mode to compare\nsparse-checkouts with the two index types (full and sparse).\n\nThanks, -Stolee\n\nDerrick Stolee (9):\n  cache-tree: clean up cache_tree_update()\n  cache-tree: extract subtree_pos()\n  fsmonitor: de-duplicate BUG()s around dirty bits\n  repository: add repo reference to index_state\n  name-hash: use trace2 regions for init\n  sparse-checkout: load sparse-checkout patterns\n  sparse-checkout: hold pattern list in index\n  test-lib: test_region looks for trace2 regions\n  t1092: test interesting sparse-checkout scenarios\n\n builtin/sparse-checkout.c                |  22 +-\n cache-tree.c                             |  20 +-\n cache-tree.h                             |   2 +\n cache.h                                  |   3 +\n dir.c                                    |  17 ++\n dir.h                                    |   2 +\n fsmonitor.c                              |  27 +-\n name-hash.c                              |   3 +\n repository.c                             |   4 +\n t/t0500-progress-display.sh              |   3 +-\n t/t1092-sparse-checkout-compatibility.sh | 323 +++++++++++++++++++++++\n t/test-lib-functions.sh                  |  40 +++\n unpack-trees.c                           |   6 +-\n 13 files changed, 431 insertions(+), 41 deletions(-)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\n\nbase-commit: a4b6d202caad83c6dc29abe9b17e53a1b3fb54a0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-839%2Fderrickstolee%2Fmore-index-cleanups-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-839/derrickstolee/more-index-cleanups-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/839\n-- \ngitgitgadget\n"},{"id":"414820","messageId":"a6f2406a79512f517a1bc2700f806c546bbe9ed6.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 2/9] cache-tree: extract subtree_pos()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:52Z","receivedAt":"2021-01-20T16:57:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThis method will be helpful to use outside of cache-tree.c in a later\nfeature. The implementation is subtle due to subtree_name_cmp() sorting\nby length and then lexicographically.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 6 +++---\n cache-tree.h | 2 ++\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c1e49901c17..2b130dd5e19 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,\n \treturn memcmp(one, two, onelen);\n }\n \n-static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n {\n \tstruct cache_tree_sub **down = it->down;\n \tint lo, hi;\n@@ -72,7 +72,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,\n \t\t\t\t\t   int create)\n {\n \tstruct cache_tree_sub *down;\n-\tint pos = subtree_pos(it, path, pathlen);\n+\tint pos = cache_tree_subtree_pos(it, path, pathlen);\n \tif (0 <= pos)\n \t\treturn it->down[pos];\n \tif (!create)\n@@ -123,7 +123,7 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)\n \tit->entry_count = -1;\n \tif (!*slash) {\n \t\tint pos;\n-\t\tpos = subtree_pos(it, path, namelen);\n+\t\tpos = cache_tree_subtree_pos(it, path, namelen);\n \t\tif (0 <= pos) {\n \t\t\tcache_tree_free(&it->down[pos]->cache_tree);\n \t\t\tfree(it->down[pos]);\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 639bfa5340e..8efeccebfc9 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -27,6 +27,8 @@ void cache_tree_free(struct cache_tree **);\n void cache_tree_invalidate_path(struct index_state *, const char *);\n struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);\n \n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);\n+\n void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n-- \ngitgitgadget\n\n"},{"id":"414821","messageId":"555e210dc03e3966d39b84d5fd5d409038422c40.1611161639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH 9/9] t1092: test interesting sparse-checkout scenarios","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-20T16:53:59Z","receivedAt":"2021-01-20T17:01:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThese also document some behaviors that differ from a full checkout, and\npossibly in a way that is not intended.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t1092-sparse-checkout-compatibility.sh | 323 +++++++++++++++++++++++\n 1 file changed, 323 insertions(+)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nnew file mode 100755\nindex 00000000000..46f9dc2cdf3\n--- /dev/null\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -0,0 +1,323 @@\n+#!/bin/sh\n+\n+test_description='compare full workdir to sparse workdir'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init initial-repo &&\n+\t(\n+\t\tcd initial-repo &&\n+\t\techo a >a &&\n+\t\techo \"after deep\" >e &&\n+\t\techo \"after folder1\" >g &&\n+\t\tmkdir folder1 folder2 deep x &&\n+\t\tmkdir deep/deeper1 deep/deeper2 &&\n+\t\tmkdir deep/deeper1/deepest &&\n+\t\techo \"after deeper1\" >deep/e &&\n+\t\techo \"after deepest\" >deep/deeper1/e &&\n+\t\tcp a folder1 &&\n+\t\tcp a folder2 &&\n+\t\tcp a deep &&\n+\t\tcp a deep/deeper1 &&\n+\t\tcp a deep/deeper2 &&\n+\t\tcp a deep/deeper1/deepest &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit checkout -b base &&\n+\t\tfor dir in folder1 folder2 deep\n+\t\tdo\n+\t\t\tgit checkout -b update-$dir &&\n+\t\t\techo \"updated $dir\" >$dir/a &&\n+\t\t\tgit commit -a -m \"update $dir\" || return 1\n+\t\tdone &&\n+\n+\t\tgit checkout -b rename-base base &&\n+\t\techo >folder1/larger-content <<-\\EOF &&\n+\t\tmatching\n+\t\tlines\n+\t\thelp\n+\t\tinexact\n+\t\trenames\n+\t\tEOF\n+\t\tcp folder1/larger-content folder2/ &&\n+\t\tcp folder1/larger-content deep/deeper1/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"add interesting rename content\" &&\n+\n+\t\tgit checkout -b rename-out-to-out rename-base &&\n+\t\tmv folder1/a folder2/b &&\n+\t\tmv folder1/larger-content folder2/edited-content &&\n+\t\techo >>folder2/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to folder2/...\" &&\n+\n+\t\tgit checkout -b rename-out-to-in rename-base &&\n+\t\tmv folder1/a deep/deeper1/b &&\n+\t\tmv folder1/larger-content deep/deeper1/edited-content &&\n+\t\techo >>deep/deeper1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to deep/deeper1/...\" &&\n+\n+\t\tgit checkout -b rename-in-to-out rename-base &&\n+\t\tmv deep/deeper1/a folder1/b &&\n+\t\tmv deep/deeper1/larger-content folder1/edited-content &&\n+\t\techo >>folder1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename deep/deeper1/... to folder1/...\" &&\n+\n+\t\tgit checkout -b deepest base &&\n+\t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n+\t\tgit commit -a -m \"update deepest\" &&\n+\n+\t\tgit checkout -f base &&\n+\t\tgit reset --hard\n+\t)\n+'\n+\n+init_repos () {\n+\trm -rf full-checkout sparse-checkout sparse-index &&\n+\n+\t# create repos in initial state\n+\tcp -r initial-repo full-checkout &&\n+\tgit -C full-checkout reset --hard &&\n+\n+\tcp -r initial-repo sparse-checkout &&\n+\tgit -C sparse-checkout reset --hard &&\n+\tgit -C sparse-checkout sparse-checkout init --cone &&\n+\n+\t# initialize sparse-checkout definitions\n+\tgit -C sparse-checkout sparse-checkout set deep\n+}\n+\n+run_on_sparse () {\n+\t(\n+\t\tcd sparse-checkout &&\n+\t\t$* >../sparse-checkout-out 2>../sparse-checkout-err\n+\t)\n+}\n+\n+run_on_all () {\n+\t(\n+\t\tcd full-checkout &&\n+\t\t$* >../full-checkout-out 2>../full-checkout-err\n+\t) &&\n+\trun_on_sparse $*\n+}\n+\n+test_all_match () {\n+\trun_on_all $* &&\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err\n+}\n+\n+test_expect_success 'status with options' '\n+\tinit_repos &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\trun_on_all \"touch README.md\" &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno\n+'\n+\n+test_expect_success 'add, commit, checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>README.md\n+\tEOF\n+\trun_on_all \"../edit-contents\" &&\n+\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Add README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents\" &&\n+\n+\ttest_all_match git add -A &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Extend README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout -\n+'\n+\n+test_expect_success 'add, commit, checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Add README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add -A &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Extend README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents deep/newfile\" &&\n+\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git add . &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"add deep/newfile\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout -\n+'\n+\n+test_expect_success 'checkout and reset --hard' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout update-deep &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout -b reset-test &&\n+\ttest_all_match git reset --hard deepest &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\ttest_all_match git reset --hard update-folder2\n+'\n+\n+test_expect_success 'diff --staged' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>README.md\n+\tEOF\n+\trun_on_all \"../edit-contents\" &&\n+\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged\n+'\n+\n+test_expect_success 'diff with renames' '\n+\tinit_repos &&\n+\n+\tfor branch in rename-out-to-out rename-out-to-in rename-in-to-out\n+\tdo\n+\t\ttest_all_match git checkout rename-base &&\n+\t\ttest_all_match git checkout $branch -- .&&\n+\t\ttest_all_match git diff --staged &&\n+\t\ttest_all_match git diff --staged --find-renames || return 1\n+\tdone\n+'\n+\n+test_expect_success 'log with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git log -- a &&\n+\ttest_all_match git log -- folder1/a &&\n+\ttest_all_match git log -- folder2/a &&\n+\ttest_all_match git log -- deep/a &&\n+\ttest_all_match git log -- deep/deeper1/a &&\n+\ttest_all_match git log -- deep/deeper1/deepest/a &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git log -- folder1/a\n+'\n+\n+test_expect_success 'blame with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame a &&\n+\ttest_all_match git blame deep/a &&\n+\ttest_all_match git blame deep/deeper1/a &&\n+\ttest_all_match git blame deep/deeper1/deepest/a\n+'\n+\n+# TODO: blame currently does not support blaming files outside of the\n+# sparse definition. It complains that the file doesn't exist locally.\n+test_expect_failure 'blame with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame folder1/a &&\n+\ttest_all_match git blame folder2/a &&\n+\ttest_all_match git blame deep/deeper2/a &&\n+\ttest_all_match git blame deep/deeper2/deepest/a\n+'\n+\n+# TODO: reset currently does not behave as expected when in a\n+# sparse-checkout.\n+test_expect_failure 'checkout and reset (mixed)' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b reset-test update-deep &&\n+\ttest_all_match git reset deepest &&\n+\ttest_all_match git reset update-folder1 &&\n+\ttest_all_match git reset update-folder2\n+'\n+\n+test_expect_success 'merge' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge update-deep &&\n+\ttest_all_match git merge -m \"folder1\" update-folder1 &&\n+\ttest_all_match git rev-parse HEAD^{tree} &&\n+\ttest_all_match git merge -m \"folder2\" update-folder2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n+test_expect_success 'merge with outside renames' '\n+\tinit_repos &&\n+\n+\tfor type in out-to-out out-to-in in-to-out\n+\tdo\n+\t\ttest_all_match git reset --hard &&\n+\t\ttest_all_match git checkout -f -b merge-$type update-deep &&\n+\t\ttest_all_match git merge -m \"$type\" rename-$type &&\n+\t\ttest_all_match git rev-parse HEAD^{tree} || return 1\n+\tdone\n+'\n+\n+test_expect_success 'clean' '\n+\tinit_repos &&\n+\n+\techo bogus >>.gitignore &&\n+\trun_on_all cp ../.gitignore . &&\n+\ttest_all_match git add .gitignore &&\n+\ttest_all_match git commit -m ignore-bogus-files &&\n+\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all touch folder1/bogus &&\n+\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git clean -f &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xdf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_path_is_dir sparse-checkout/folder1\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"414822","messageId":"CABPp-BFiNvPaAffQ0uBfpkHY1WM2gR3ssvpwzxpDhrZmZZjiDA@mail.gmail.com","threadId":"55018","inReplyTo":"0bccfd34ae5924aef0432fd6727debb75c052da5.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/9] cache-tree: clean up cache_tree_update()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:21:08Z","receivedAt":"2021-01-20T17:23:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Make the method safer by allocating a cache_tree member for the given\n> index_state if it is not already present.\n>\n> Also drop local variables that are used exactly once and can be found\n> directly from the 'istate' parameter.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  cache-tree.c | 14 +++++++++-----\n>  1 file changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 3f1a8d4f1b7..c1e49901c17 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,\n>\n>  int cache_tree_update(struct index_state *istate, int flags)\n>  {\n> -       struct cache_tree *it = istate->cache_tree;\n> -       struct cache_entry **cache = istate->cache;\n> -       int entries = istate->cache_nr;\n> -       int skip, i = verify_cache(cache, entries, flags);\n> +       int skip, i;\n> +\n> +       i = verify_cache(istate->cache, istate->cache_nr, flags);\n\nAll mechanical changes so far; these look obviously correct.\n\n>\n>         if (i)\n>                 return i;\n> +\n> +       if (!istate->cache_tree)\n> +               istate->cache_tree = cache_tree();\n\nThis is the only substantive change.  It seems fairly innocuous, but\nit makes me wonder the reasoning...I don't know/remember enough about\ncache_tree handling to know when this would or wouldn't have already\nbeen allocated.  It seems that this would have had to segfault below\nif istate->cache_tree were ever NULL, and I don't see you mentioning\nany bug you are fixing, so I presume this means you are going to be\nadding new codepaths somewhere that cause this function to be reached\nunder different circumstances than previously had been and you need it\nto be more safe for those.  Is that correct?  Or is it just an\nabundance of caution thing that you're adding?  If the latter, any\nreason you chose to allocate one rather than assume it's a violation\nof design invariants and BUG() instead?  (Perhaps the commit message\ncould add a sentence about the rationale for the extra safety?)\n\n> +\n>         trace_performance_enter();\n>         trace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> -       i = update_one(it, cache, entries, \"\", 0, &skip, flags);\n> +       i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> +                      \"\", 0, &skip, flags);\n\nAnother mechanical update; looks good.\n\n>         trace2_region_leave(\"cache_tree\", \"update\", the_repository);\n>         trace_performance_leave(\"cache_tree_update\");\n>         if (i < 0)\n> --\n> gitgitgadget\n"},{"id":"414825","messageId":"CABPp-BH7K3ddUEJ2aKqZ4D5bkNh43JNm5LuvDRbC5e2nQfVa0g@mail.gmail.com","threadId":"55018","inReplyTo":"a6f2406a79512f517a1bc2700f806c546bbe9ed6.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/9] cache-tree: extract subtree_pos()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:23:43Z","receivedAt":"2021-01-20T17:33:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> This method will be helpful to use outside of cache-tree.c in a later\n> feature. The implementation is subtle due to subtree_name_cmp() sorting\n> by length and then lexicographically.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  cache-tree.c | 6 +++---\n>  cache-tree.h | 2 ++\n>  2 files changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index c1e49901c17..2b130dd5e19 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,\n>         return memcmp(one, two, onelen);\n>  }\n>\n> -static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n> +int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n>  {\n>         struct cache_tree_sub **down = it->down;\n>         int lo, hi;\n> @@ -72,7 +72,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,\n>                                            int create)\n>  {\n>         struct cache_tree_sub *down;\n> -       int pos = subtree_pos(it, path, pathlen);\n> +       int pos = cache_tree_subtree_pos(it, path, pathlen);\n>         if (0 <= pos)\n>                 return it->down[pos];\n>         if (!create)\n> @@ -123,7 +123,7 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)\n>         it->entry_count = -1;\n>         if (!*slash) {\n>                 int pos;\n> -               pos = subtree_pos(it, path, namelen);\n> +               pos = cache_tree_subtree_pos(it, path, namelen);\n>                 if (0 <= pos) {\n>                         cache_tree_free(&it->down[pos]->cache_tree);\n>                         free(it->down[pos]);\n> diff --git a/cache-tree.h b/cache-tree.h\n> index 639bfa5340e..8efeccebfc9 100644\n> --- a/cache-tree.h\n> +++ b/cache-tree.h\n> @@ -27,6 +27,8 @@ void cache_tree_free(struct cache_tree **);\n>  void cache_tree_invalidate_path(struct index_state *, const char *);\n>  struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);\n>\n> +int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);\n> +\n>  void cache_tree_write(struct strbuf *, struct cache_tree *root);\n>  struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n>\n> --\n> gitgitgadget\n\nSimple, straight-forward patch for exposing the function outside the\nfile scope; looks good.\n"},{"id":"414826","messageId":"CABPp-BEL=tjCx_x1z0NpNTmt3q9_CHtufwWg9FtqKBWWub00UQ@mail.gmail.com","threadId":"55018","inReplyTo":"838922de2e9756c00f4a159b2b8722ae4d28b011.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/9] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:26:38Z","receivedAt":"2021-01-20T17:33:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The index has an fsmonitor_dirty bitmap that records which index entries\n> are \"dirty\" based on the response from the FSMonitor. If this bitmap\n> ever grows larger than the index, then there was an error in how it was\n> constructed, and it was probably a developer's bug.\n>\n> There are several BUG() statements that are very similar, so replace\n> these uses with a simpler assert_index_minimum(). Since there is one\n> caller that uses a custom 'pos' value instead of the bit_size member, we\n> cannot simplify it too much. However, the error string is identical in\n> each, so this simplifies things.\n>\n> The end result is that the code is simpler to read while also preserving\n> these assertions for developers in the FSMonitor space.\n\nIndeed, looking through the patch, the end result is simpler to read.\nNice cleanup.\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  fsmonitor.c | 27 +++++++++++++--------------\n>  1 file changed, 13 insertions(+), 14 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index ca031c3abb8..52a50a9545a 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -13,14 +13,19 @@\n>\n>  struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n>\n> +static void assert_index_minimum(struct index_state *istate, size_t pos)\n> +{\n> +       if (pos > istate->cache_nr)\n> +               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> +                   (uintmax_t)pos, istate->cache_nr);\n> +}\n> +\n>  static void fsmonitor_ewah_callback(size_t pos, void *is)\n>  {\n>         struct index_state *istate = (struct index_state *)is;\n>         struct cache_entry *ce;\n>\n> -       if (pos >= istate->cache_nr)\n> -               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n> -                   (uintmax_t)pos, istate->cache_nr);\n> +       assert_index_minimum(istate, pos);\n>\n>         ce = istate->cache[pos];\n>         ce->ce_flags &= ~CE_FSMONITOR_VALID;\n> @@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n>         }\n>         istate->fsmonitor_dirty = fsmonitor_dirty;\n>\n> -       if (!istate->split_index &&\n> -           istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +       if (!istate->split_index)\n> +               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>\n>         trace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n>         return 0;\n> @@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n>         uint32_t ewah_size = 0;\n>         int fixup = 0;\n>\n> -       if (!istate->split_index &&\n> -           istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +       if (!istate->split_index)\n> +               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>\n>         put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n>         strbuf_add(sb, &hdr_version, sizeof(uint32_t));\n> @@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)\n>                         }\n>\n>                         /* Mark all previously saved entries as dirty */\n> -                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -                               BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -                                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +                       assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>                         ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n>\n>                         refresh_fsmonitor(istate);\n> --\n> gitgitgadget\n>\n"},{"id":"414827","messageId":"CABPp-BGOVUDqDvTi0VOK7fD1EeFrhjCfZm241BqgA5_yMDH-VQ@mail.gmail.com","threadId":"55018","inReplyTo":"d4ff0468fc0e22fe3ef82f74603ac781fd7711e1.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/9] repository: add repo reference to index_state","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:46:05Z","receivedAt":"2021-01-20T17:48:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> It will be helpful to add behavior to index opertations that might\n\ns/opertations/operations/\n\n> trigger an object lookup. Since each index belongs to a specific\n> repository, add a 'repo' pointer to struct index_state that allows\n> access to this repository.\n>\n> This will prevent future changes from needing to pass an additional\n> 'struct repository *repo' parameter and instead rely only on the 'struct\n> index_state *istate' parameter.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  cache.h      | 1 +\n>  repository.c | 4 ++++\n>  2 files changed, 5 insertions(+)\n>\n> diff --git a/cache.h b/cache.h\n> index 71097657489..f9c7a603841 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -328,6 +328,7 @@ struct index_state {\n>         struct ewah_bitmap *fsmonitor_dirty;\n>         struct mem_pool *ce_mem_pool;\n>         struct progress *progress;\n> +       struct repository *repo;\n>  };\n>\n>  /* Name hashing */\n> diff --git a/repository.c b/repository.c\n> index a4174ddb062..67a4c1da2d9 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)\n>         if (!repo->index)\n>                 repo->index = xcalloc(1, sizeof(*repo->index));\n>\n> +       /* Complete the double-reference */\n> +       if (!repo->index->repo)\n> +               repo->index->repo = repo;\n> +\n>         return read_index_from(repo->index, repo->index_file, repo->gitdir);\n>  }\n>\n> --\n> gitgitgadget\n\nSince we have repo->index and we have index->repo, which are intended\nto be circular...what if they aren't?  Do we want or need to add\nassertions anywhere that repo == repo->index->repo or that index ==\nindex->repo->index ?\n\nMy initial implementations of --remerge-diff[1] played around with\ncreating a second repo, with a different primary object store but\neverything else the same.  The index for the two repository objects\nwas thus the same, and thus clearly would have violated this assumed\ninvariant for one of the two repos.  I discarded that initial\nimplementation (which I didn't quite have working) because I\ndiscovered tmp-objdir.h and was able to add some\ntmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions\nthat merely altered the existing repo's primary object store, but I'm\ncurious if there might be other cases of folks doing stuff that might\nhave weird failures with this new invariant.\n\nIt's entirely possible that --remerge-diff was just so different, and\nI was so unfamiliar with repo objects (and still kind of am) that I\nwas just doing weird stuff no one has done before, so perhaps no\nadditional checks are needed -- I'm just throwing my gut question out\nthere as food for thought.\n\n\n\n[1] I have not yet submitted `--remerge-diff` to the list; you haven't\nmissed anything.  I'm waiting for merge-ort to be submitted, reviewed,\nand merged first.  It's the remerge-diff branch in my fork on GitHub\nif anyone is curious, though.\n"},{"id":"414828","messageId":"CABPp-BEhJDd=Om49Cpb2CuuHn_UBuNaxJjGKZEhWfY3r3vcefA@mail.gmail.com","threadId":"55018","inReplyTo":"3ba4b35f09cf3dc313787cdec6856286fe97d8a8.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/9] name-hash: use trace2 regions for init","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:47:50Z","receivedAt":"2021-01-20T17:49:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The lazy_init_name_hash() populates a hashset with all filenames and\n> another with all directories represented in the index. This is run only\n> if we need to use the hashsets to check for existence or case-folding\n> renames.\n>\n> Place trace2 regions where there is already a performance trace.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  name-hash.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/name-hash.c b/name-hash.c\n> index 5d3c7b12c18..4e03fac9bb1 100644\n> --- a/name-hash.c\n> +++ b/name-hash.c\n> @@ -7,6 +7,7 @@\n>   */\n>  #include \"cache.h\"\n>  #include \"thread-utils.h\"\n> +#include \"trace2.h\"\n>\n>  struct dir_entry {\n>         struct hashmap_entry ent;\n> @@ -577,6 +578,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n>         if (istate->name_hash_initialized)\n>                 return;\n>         trace_performance_enter();\n> +       trace2_region_enter(\"index\", \"name-hash-init\", istate->repo);\n>         hashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n>         hashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);\n>\n> @@ -597,6 +599,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n>         }\n>\n>         istate->name_hash_initialized = 1;\n> +       trace2_region_leave(\"index\", \"name-hash-init\", istate->repo);\n>         trace_performance_leave(\"initialize name hash\");\n>  }\n>\n> --\n> gitgitgadget\n\nYaay for trace2.  :-)\n\nTotal sidenote: Are we ever going to get rid of the trace1 stuff?  I'm\nsure this was discussed somewhere, but I don't know where.\n"},{"id":"414829","messageId":"CABPp-BHSssDCvyC3rGZasR8yWxJYN9Y09LZL2BBd2hyFgG08Vw@mail.gmail.com","threadId":"55018","inReplyTo":"64358ec7ea2b3df4a8f1099452579a7568996921.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/9] sparse-checkout: load sparse-checkout patterns","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T17:54:18Z","receivedAt":"2021-01-20T18:00:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> A future feature will want to load the sparse-checkout patterns into a\n> pattern_list, but the current mechanism to do so is a bit complicated.\n> This is made difficult due to needing to find the sparse-checkout file\n> in different ways throughout the codebase.\n>\n> The logic implemented in the new get_sparse_checkout_patterns() was\n> duplicated in populate_from_existing_patterns() in unpack-trees.c. Use\n> the new method instead, keeping the logic around handling the struct\n> unpack_trees_options.\n>\n> The callers to get_sparse_checkout_filename() in\n> builtin/sparse-checkout.c manipulate the sparse-checkout file directly,\n> so it is not appropriate to replace logic in that file with\n> get_sparse_checkout_patterns().\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  builtin/sparse-checkout.c |  5 -----\n>  dir.c                     | 17 +++++++++++++++++\n>  dir.h                     |  2 ++\n>  unpack-trees.c            |  6 +-----\n>  4 files changed, 20 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index e3140db2a0a..2306a9ad98e 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {\n>         NULL\n>  };\n>\n> -static char *get_sparse_checkout_filename(void)\n> -{\n> -       return git_pathdup(\"info/sparse-checkout\");\n> -}\n> -\n>  static void write_patterns_to_file(FILE *fp, struct pattern_list *pl)\n>  {\n>         int i;\n> diff --git a/dir.c b/dir.c\n> index d637461da5c..d153a63bbd1 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2998,6 +2998,23 @@ void setup_standard_excludes(struct dir_struct *dir)\n>         }\n>  }\n>\n> +char *get_sparse_checkout_filename(void)\n> +{\n> +       return git_pathdup(\"info/sparse-checkout\");\n> +}\n> +\n> +int get_sparse_checkout_patterns(struct pattern_list *pl)\n> +{\n> +       int res;\n> +       char *sparse_filename = get_sparse_checkout_filename();\n> +\n> +       pl->use_cone_patterns = core_sparse_checkout_cone;\n> +       res = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL);\n> +\n> +       free(sparse_filename);\n> +       return res;\n> +}\n> +\n>  int remove_path(const char *name)\n>  {\n>         char *slash;\n> diff --git a/dir.h b/dir.h\n> index a3c40dec516..facfae47402 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -448,6 +448,8 @@ int is_empty_dir(const char *dir);\n>\n>  void setup_standard_excludes(struct dir_struct *dir);\n>\n> +char *get_sparse_checkout_filename(void);\n> +int get_sparse_checkout_patterns(struct pattern_list *pl);\n>\n>  /* Constants for remove_dir_recursively: */\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index af6e9b9c2fd..837b8bb42fb 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1549,14 +1549,10 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n>  static void populate_from_existing_patterns(struct unpack_trees_options *o,\n>                                             struct pattern_list *pl)\n>  {\n> -       char *sparse = git_pathdup(\"info/sparse-checkout\");\n> -\n> -       pl->use_cone_patterns = core_sparse_checkout_cone;\n> -       if (add_patterns_from_file_to_list(sparse, \"\", 0, pl, NULL) < 0)\n> +       if (get_sparse_checkout_patterns(pl) < 0)\n>                 o->skip_sparse_checkout = 1;\n>         else\n>                 o->pl = pl;\n> -       free(sparse);\n>  }\n>\n>\n> --\n> gitgitgadget\n\nLooks straightforward and well motivated to me.\n\nBut the cherry on top that really sells this patch is that more lines\nof dir.c will blame to someone besides me.  Win-win!\n"},{"id":"414831","messageId":"CABPp-BEeDj4v4t8w6giy-yo8ET0mnD3es95aGQJvw7g8_jkjzA@mail.gmail.com","threadId":"55018","inReplyTo":"91344f5108c835a48b2460b9de75c489045b9cce.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 7/9] sparse-checkout: hold pattern list in index","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T18:03:20Z","receivedAt":"2021-01-20T18:06:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> As we modify the sparse-checkout definition, we perform index operations\n> on a pattern_list that only exists in-memory. This allows easy backing\n> out in case the index update fails.\n>\n> However, if the index write itself cares about the sparse-checkout\n> pattern set, we need access to that in-memory copy. Place a pointer to\n> a 'struct pattern_list' in the index so we can access this on-demand.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  builtin/sparse-checkout.c | 17 ++++++++++-------\n>  cache.h                   |  2 ++\n>  2 files changed, 12 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 2306a9ad98e..e00b82af727 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)\n>         if (is_index_unborn(r->index))\n>                 return UPDATE_SPARSITY_SUCCESS;\n>\n> +       r->index->sparse_checkout_patterns = pl;\n> +\n>         memset(&o, 0, sizeof(o));\n>         o.verbose_update = isatty(2);\n>         o.update = 1;\n> @@ -138,6 +140,7 @@ static int update_working_directory(struct pattern_list *pl)\n>         else\n>                 rollback_lock_file(&lock_file);\n>\n> +       r->index->sparse_checkout_patterns = NULL;\n>         return result;\n\nThe setting back to NULL made me curious; we don't want this\ninformation to remain available later?  Is it only going to be used\nfor the updating of the working directory?\n\nI dug a bit into the callers, and didn't find the answer to my\nquestion...but I did notice that modify_pattern_list() will correctly\nfree the patterns after write_patterns_and_update() via calling\nclear_pattern_list(&pl), but sparse_checkout_init() appears to leak\nthe patterns it allocates.  That's a separate issue from this patch,\nbut do you want to fix that up while working in this area (so I avoid\nstepping on your toes with all your other patches)?\n\n>  }\n>\n> @@ -517,19 +520,18 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)\n>  {\n>         int result;\n>         int changed_config = 0;\n> -       struct pattern_list pl;\n> -       memset(&pl, 0, sizeof(pl));\n> +       struct pattern_list *pl = xcalloc(1, sizeof(*pl));\n>\n>         switch (m) {\n>         case ADD:\n>                 if (core_sparse_checkout_cone)\n> -                       add_patterns_cone_mode(argc, argv, &pl);\n> +                       add_patterns_cone_mode(argc, argv, pl);\n>                 else\n> -                       add_patterns_literal(argc, argv, &pl);\n> +                       add_patterns_literal(argc, argv, pl);\n>                 break;\n>\n>         case REPLACE:\n> -               add_patterns_from_input(&pl, argc, argv);\n> +               add_patterns_from_input(pl, argc, argv);\n\nSlightly annoying that the other functions are (argc, argv, pl) and\nthis one is (pl, argc, argv).  But again, that's outside the scope of\nthis patch and might not be worth the churn to fix.\n\n>                 break;\n>         }\n>\n> @@ -539,12 +541,13 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)\n>                 changed_config = 1;\n>         }\n>\n> -       result = write_patterns_and_update(&pl);\n> +       result = write_patterns_and_update(pl);\n>\n>         if (result && changed_config)\n>                 set_config(MODE_NO_PATTERNS);\n>\n> -       clear_pattern_list(&pl);\n> +       clear_pattern_list(pl);\n> +       free(pl);\n>         return result;\n>  }\n>\n> diff --git a/cache.h b/cache.h\n> index f9c7a603841..bf4ec3de4b0 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -305,6 +305,7 @@ static inline unsigned int canon_mode(unsigned int mode)\n>  struct split_index;\n>  struct untracked_cache;\n>  struct progress;\n> +struct pattern_list;\n>\n>  struct index_state {\n>         struct cache_entry **cache;\n> @@ -329,6 +330,7 @@ struct index_state {\n>         struct mem_pool *ce_mem_pool;\n>         struct progress *progress;\n>         struct repository *repo;\n> +       struct pattern_list *sparse_checkout_patterns;\n>  };\n>\n>  /* Name hashing */\n> --\n> gitgitgadget\n\nOtherwise, this patch looks good to me.\n"},{"id":"414833","messageId":"CABPp-BGrtcQ1pDCB8_cV77dqxige0v8rk4tqFRXe=HOUsn2DNw@mail.gmail.com","threadId":"55018","inReplyTo":"8326a9b5320e1e774caef568fcce2bfd2ec13cb1.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 8/9] test-lib: test_region looks for trace2 regions","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T18:20:58Z","receivedAt":"2021-01-20T18:32:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Most test cases can verify Git's behavior using input/output\n> expectations or changes to the .git directory. However, sometimes we\n> want to check that Git did or did not run a certain section of code.\n> This is particularly important for performance-only features that we\n> want to ensure have been enabled in certain cases.\n>\n> Add a new 'test_region' function that checks if a trace2 region was\n> entered and left in a given trace2 event log.\n\nOoh, others do this too?  Sounds like a helpful function to add, but\njust checking for entered and left means that...\n\n>\n> There is one existing test (t0500-progress-display.sh) that performs\n> this check already, so use the helper function instead. More uses will\n> be added in a later change.\n>\n> t6423-merge-rename-directories.sh also greps for region_enter lines, but\n> it verifies the number of such lines, which is not the same as an\n> existence check.\n\n...yeah, won't cover the case that I added.  That's fine, since it\nappears to be a one-off for now and we don't know of any other cases,\ncurrent or planned, that want to do something like that yet.\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  t/t0500-progress-display.sh |  3 +--\n>  t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++\n>  2 files changed, 41 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index 1ed1df351cb..c461b89dfaf 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '\n>                 \"Working hard\" <in 2>stderr &&\n>\n>         # t0212/parse_events.perl intentionally omits regions and data.\n> -       grep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> -       grep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +       test_region category progress trace.event &&\n\nSidenote: Hmm...about 40% of my region labels in merge-ort.c and 90%\nin diffcore-rename.c have spaces in them.  This function could still\nbe used, but I'm curious if I should change the labels (but then\nagain, they are testing logical regions rather than individual\nfunctions, and the spaces instead of underscores kind of convey\nthat...)\n\n>         grep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n>         grep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n>  '\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 999982fe4a9..c878db93013 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1655,3 +1655,43 @@ test_subcommand () {\n>                 grep \"\\[$expr\\]\"\n>         fi\n>  }\n> +\n> +# Check that the given command was invoked as part of the\n> +# trace2-format trace on stdin.\n> +#\n> +#      test_region [!] <category> <label> git <command> <args>...\n> +#\n> +# For example, to look for trace2_region_enter(\"index\", \"do_read_index\", repo)\n> +# in an invocation of \"git checkout HEAD~1\", run\n> +#\n> +#      GIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n> +#              git checkout HEAD~1 &&\n> +#      test_region index do_read_index <trace.txt\n> +#\n> +# If the first parameter passed is !, this instead checks that\n> +# the given region was not entered.\n> +#\n> +test_region () {\n> +       local expect_exit=0\n> +       if test \"$1\" = \"!\"\n> +       then\n> +               expect_exit=1\n> +               shift\n> +       fi\n> +\n> +       grep -e \"region_enter\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n> +       exitcode=$?\n> +\n> +       if test $exitcode != $expect_exit\n> +       then\n> +               return 1\n> +       fi\n> +\n> +       grep -e \"region_leave\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n> +       exitcode=$?\n> +\n> +       if test $exitcode != $expect_exit\n> +       then\n> +               return 1\n> +       fi\n> +}\n> --\n> gitgitgadget\n\nThis patch looks good to me.\n"},{"id":"414838","messageId":"bbed0d73-fa9d-92a0-3bc4-1d44263bb2fb@gmail.com","threadId":"55018","inReplyTo":"CABPp-BFiNvPaAffQ0uBfpkHY1WM2gR3ssvpwzxpDhrZmZZjiDA@mail.gmail.com","subject":"Re: [PATCH 1/9] cache-tree: clean up cache_tree_update()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-20T19:10:30Z","receivedAt":"2021-01-20T19:11:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/20/2021 12:21 PM, Elijah Newren wrote:\n> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:...\n>> +\n>> +       if (!istate->cache_tree)\n>> +               istate->cache_tree = cache_tree();\n> \n> This is the only substantive change.  It seems fairly innocuous, but\n> it makes me wonder the reasoning...I don't know/remember enough about\n> cache_tree handling to know when this would or wouldn't have already\n> been allocated.  It seems that this would have had to segfault below\n> if istate->cache_tree were ever NULL, and I don't see you mentioning\n> any bug you are fixing, so I presume this means you are going to be\n> adding new codepaths somewhere that cause this function to be reached\n> under different circumstances than previously had been and you need it\n> to be more safe for those.  Is that correct?  Or is it just an\n> abundance of caution thing that you're adding?  If the latter, any\n> reason you chose to allocate one rather than assume it's a violation\n> of design invariants and BUG() instead?  (Perhaps the commit message\n> could add a sentence about the rationale for the extra safety?)\n\nIt's something I need in the future when I use the cache_tree_update()\nin more places. I think I call it two times, and either I need to\ninitialize the cache_tree member outside of both, or just make it a\nfeature of the method that it will re-initialize the cache-tree.\n\nNote: the implementation treats an initialized, but empty cache-tree\nas \"invalid\" so update_one() correctly populates the full tree.\n\nThanks,\n-Stolee\n\n"},{"id":"414841","messageId":"f790db9c-0c6d-3661-93d9-0339e22c12ff@gmail.com","threadId":"55018","inReplyTo":"CABPp-BGOVUDqDvTi0VOK7fD1EeFrhjCfZm241BqgA5_yMDH-VQ@mail.gmail.com","subject":"Re: [PATCH 4/9] repository: add repo reference to index_state","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-20T19:16:51Z","receivedAt":"2021-01-20T19:18:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/20/2021 12:46 PM, Elijah Newren wrote:\n> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> It will be helpful to add behavior to index opertations that might\n> \n> s/opertations/operations/\n\nThanks.\n\n>> trigger an object lookup. Since each index belongs to a specific\n>> repository, add a 'repo' pointer to struct index_state that allows\n>> access to this repository.\n>>\n>> This will prevent future changes from needing to pass an additional\n>> 'struct repository *repo' parameter and instead rely only on the 'struct\n>> index_state *istate' parameter.\n>>\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>>  cache.h      | 1 +\n>>  repository.c | 4 ++++\n>>  2 files changed, 5 insertions(+)\n>>\n>> diff --git a/cache.h b/cache.h\n>> index 71097657489..f9c7a603841 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -328,6 +328,7 @@ struct index_state {\n>>         struct ewah_bitmap *fsmonitor_dirty;\n>>         struct mem_pool *ce_mem_pool;\n>>         struct progress *progress;\n>> +       struct repository *repo;\n>>  };\n>>\n>>  /* Name hashing */\n>> diff --git a/repository.c b/repository.c\n>> index a4174ddb062..67a4c1da2d9 100644\n>> --- a/repository.c\n>> +++ b/repository.c\n>> @@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)\n>>         if (!repo->index)\n>>                 repo->index = xcalloc(1, sizeof(*repo->index));\n>>\n>> +       /* Complete the double-reference */\n>> +       if (!repo->index->repo)\n>> +               repo->index->repo = repo;\n>> +\n>>         return read_index_from(repo->index, repo->index_file, repo->gitdir);\n>>  }\n>>\n>> --\n>> gitgitgadget\n> \n> Since we have repo->index and we have index->repo, which are intended\n> to be circular...what if they aren't?  Do we want or need to add\n> assertions anywhere that repo == repo->index->repo or that index ==\n> index->repo->index ?\n\nHere, we are pairing them together and the loop is complete. I don't\nview that as a permanent thing. This only initializes istate->repo\nwhen we are parsing an index from a file, but not when we create one\nin memory.\n\nI imagine it will be likely in some cases to have multiple index_state\ninstances for a single repository. However, having the pointer \"this\nindex belongs to this repository\" seems helpful (to me).\n\n> My initial implementations of --remerge-diff[1] played around with\n> creating a second repo, with a different primary object store but\n> everything else the same.  The index for the two repository objects\n> was thus the same, and thus clearly would have violated this assumed\n> invariant for one of the two repos.  I discarded that initial\n> implementation (which I didn't quite have working) because I\n> discovered tmp-objdir.h and was able to add some\n> tmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions\n> that merely altered the existing repo's primary object store, but I'm\n> curious if there might be other cases of folks doing stuff that might\n> have weird failures with this new invariant.\n\nThis is an interesting concept, and definitely violates my expectations\nthat an index belongs to only one repository. I'd need to know more\nabout why this was a good design decision before being convinced that\nthe relationship should not be many-to-one (index-to-repo).\n\n> It's entirely possible that --remerge-diff was just so different, and\n> I was so unfamiliar with repo objects (and still kind of am) that I\n> was just doing weird stuff no one has done before, so perhaps no\n> additional checks are needed -- I'm just throwing my gut question out\n> there as food for thought.\n> \n> [1] I have not yet submitted `--remerge-diff` to the list; you haven't\n> missed anything.  I'm waiting for merge-ort to be submitted, reviewed,\n> and merged first.  It's the remerge-diff branch in my fork on GitHub\n> if anyone is curious, though.\n \nI'm interested in what others might say about this idea. I'd be able\nto do most of what I want to do without this patch, but it just gets\na lot messier. (istate->repo is used in the very next patch in a way\nthat would be less clean without it.)\n\nThanks,\n-Stolee\n"},{"id":"414842","messageId":"fb1800fb-474a-e20b-5596-6bb3bc22fe99@gmail.com","threadId":"55018","inReplyTo":"CABPp-BEeDj4v4t8w6giy-yo8ET0mnD3es95aGQJvw7g8_jkjzA@mail.gmail.com","subject":"Re: [PATCH 7/9] sparse-checkout: hold pattern list in index","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-20T19:22:33Z","receivedAt":"2021-01-20T19:24:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/20/2021 1:03 PM, Elijah Newren wrote:\n> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> As we modify the sparse-checkout definition, we perform index operations\n>> on a pattern_list that only exists in-memory. This allows easy backing\n>> out in case the index update fails.\n>>\n>> However, if the index write itself cares about the sparse-checkout\n>> pattern set, we need access to that in-memory copy. Place a pointer to\n>> a 'struct pattern_list' in the index so we can access this on-demand.\n>>\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>>  builtin/sparse-checkout.c | 17 ++++++++++-------\n>>  cache.h                   |  2 ++\n>>  2 files changed, 12 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n>> index 2306a9ad98e..e00b82af727 100644\n>> --- a/builtin/sparse-checkout.c\n>> +++ b/builtin/sparse-checkout.c\n>> @@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)\n>>         if (is_index_unborn(r->index))\n>>                 return UPDATE_SPARSITY_SUCCESS;\n>>\n>> +       r->index->sparse_checkout_patterns = pl;\n>> +\n>>         memset(&o, 0, sizeof(o));\n>>         o.verbose_update = isatty(2);\n>>         o.update = 1;\n>> @@ -138,6 +140,7 @@ static int update_working_directory(struct pattern_list *pl)\n>>         else\n>>                 rollback_lock_file(&lock_file);\n>>\n>> +       r->index->sparse_checkout_patterns = NULL;\n>>         return result;\n> \n> The setting back to NULL made me curious; we don't want this\n> information to remain available later?  Is it only going to be used\n> for the updating of the working directory?\n> \n> I dug a bit into the callers, and didn't find the answer to my\n> question...but I did notice that modify_pattern_list() will correctly\n> free the patterns after write_patterns_and_update() via calling\n> clear_pattern_list(&pl), but sparse_checkout_init() appears to leak\n> the patterns it allocates.  That's a separate issue from this patch,\n> but do you want to fix that up while working in this area (so I avoid\n> stepping on your toes with all your other patches)?\n\nThe thing that caught me here is that update_working_directory() uses\nan in-memory pattern_list that hasn't been committed to the\nsparse-checkout file yet. This means we need to (temporarily) point\nto this pattern_list.\n\nPerhaps this patch is premature, since nothing actually _uses_\nsparse_checkout_patterns yet. When we do add such a use, it will\ninitialize a NULL value with the patterns in the sparse-checkout\nfile. In that case, we definitely want to inject our in-memory\npatterns instead.\n\nThanks,\n-Stolee\n"},{"id":"414843","messageId":"7754da50-10bf-24cf-2fee-e1da93a7c94a@gmail.com","threadId":"55018","inReplyTo":"CABPp-BGrtcQ1pDCB8_cV77dqxige0v8rk4tqFRXe=HOUsn2DNw@mail.gmail.com","subject":"Re: [PATCH 8/9] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-20T19:24:38Z","receivedAt":"2021-01-20T19:26:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/20/2021 1:20 PM, Elijah Newren wrote:\n> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n...\n>>         # t0212/parse_events.perl intentionally omits regions and data.\n>> -       grep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n>> -       grep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n>> +       test_region category progress trace.event &&\n> \n> Sidenote: Hmm...about 40% of my region labels in merge-ort.c and 90%\n> in diffcore-rename.c have spaces in them.  This function could still\n> be used, but I'm curious if I should change the labels (but then\n> again, they are testing logical regions rather than individual\n> functions, and the spaces instead of underscores kind of convey\n> that...)\n\nYou should be able to use\n\n\ttest_region \"category with spaces\" \"progress with spaces\" trace\n\nbut if not, then the test_region helper could be improved to match.\n\nI do think that it's better to avoid spaces in these identifiers.\n\nThanks,\n-Stolee\n"},{"id":"414847","messageId":"CABPp-BH=E4bR2K21e-p+ABTG02feDF7YNvrOKhsMW5Dtet6y7Q@mail.gmail.com","threadId":"55018","inReplyTo":"555e210dc03e3966d39b84d5fd5d409038422c40.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 9/9] t1092: test interesting sparse-checkout scenarios","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T19:40:23Z","receivedAt":"2021-01-20T19:51:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> These also document some behaviors that differ from a full checkout, and\n> possibly in a way that is not intended.\n\nI'm in favor.  I should turn some of my noted weird behaviors from [1]\ninto testcases as well.\n\n[1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/\n\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  t/t1092-sparse-checkout-compatibility.sh | 323 +++++++++++++++++++++++\n>  1 file changed, 323 insertions(+)\n>  create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> new file mode 100755\n> index 00000000000..46f9dc2cdf3\n> --- /dev/null\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -0,0 +1,323 @@\n> +#!/bin/sh\n> +\n> +test_description='compare full workdir to sparse workdir'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +       git init initial-repo &&\n> +       (\n> +               cd initial-repo &&\n> +               echo a >a &&\n> +               echo \"after deep\" >e &&\n> +               echo \"after folder1\" >g &&\n> +               mkdir folder1 folder2 deep x &&\n> +               mkdir deep/deeper1 deep/deeper2 &&\n> +               mkdir deep/deeper1/deepest &&\n> +               echo \"after deeper1\" >deep/e &&\n> +               echo \"after deepest\" >deep/deeper1/e &&\n> +               cp a folder1 &&\n> +               cp a folder2 &&\n> +               cp a deep &&\n> +               cp a deep/deeper1 &&\n> +               cp a deep/deeper2 &&\n> +               cp a deep/deeper1/deepest &&\n> +               git add . &&\n> +               git commit -m \"initial commit\" &&\n> +               git checkout -b base &&\n> +               for dir in folder1 folder2 deep\n> +               do\n> +                       git checkout -b update-$dir &&\n> +                       echo \"updated $dir\" >$dir/a &&\n> +                       git commit -a -m \"update $dir\" || return 1\n> +               done &&\n> +\n> +               git checkout -b rename-base base &&\n> +               echo >folder1/larger-content <<-\\EOF &&\n> +               matching\n> +               lines\n> +               help\n> +               inexact\n> +               renames\n> +               EOF\n> +               cp folder1/larger-content folder2/ &&\n> +               cp folder1/larger-content deep/deeper1/ &&\n> +               git add . &&\n> +               git commit -m \"add interesting rename content\" &&\n> +\n> +               git checkout -b rename-out-to-out rename-base &&\n> +               mv folder1/a folder2/b &&\n> +               mv folder1/larger-content folder2/edited-content &&\n> +               echo >>folder2/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename folder1/... to folder2/...\" &&\n> +\n> +               git checkout -b rename-out-to-in rename-base &&\n> +               mv folder1/a deep/deeper1/b &&\n> +               mv folder1/larger-content deep/deeper1/edited-content &&\n> +               echo >>deep/deeper1/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename folder1/... to deep/deeper1/...\" &&\n> +\n> +               git checkout -b rename-in-to-out rename-base &&\n> +               mv deep/deeper1/a folder1/b &&\n> +               mv deep/deeper1/larger-content folder1/edited-content &&\n> +               echo >>folder1/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename deep/deeper1/... to folder1/...\" &&\n> +\n> +               git checkout -b deepest base &&\n> +               echo \"updated deepest\" >deep/deeper1/deepest/a &&\n> +               git commit -a -m \"update deepest\" &&\n> +\n> +               git checkout -f base &&\n> +               git reset --hard\n> +       )\n> +'\n> +\n> +init_repos () {\n> +       rm -rf full-checkout sparse-checkout sparse-index &&\n> +\n> +       # create repos in initial state\n> +       cp -r initial-repo full-checkout &&\n> +       git -C full-checkout reset --hard &&\n> +\n> +       cp -r initial-repo sparse-checkout &&\n> +       git -C sparse-checkout reset --hard &&\n> +       git -C sparse-checkout sparse-checkout init --cone &&\n> +\n> +       # initialize sparse-checkout definitions\n> +       git -C sparse-checkout sparse-checkout set deep\n> +}\n> +\n> +run_on_sparse () {\n> +       (\n> +               cd sparse-checkout &&\n> +               $* >../sparse-checkout-out 2>../sparse-checkout-err\n> +       )\n> +}\n> +\n> +run_on_all () {\n> +       (\n> +               cd full-checkout &&\n> +               $* >../full-checkout-out 2>../full-checkout-err\n> +       ) &&\n> +       run_on_sparse $*\n> +}\n> +\n> +test_all_match () {\n> +       run_on_all $* &&\n> +       test_cmp full-checkout-out sparse-checkout-out &&\n> +       test_cmp full-checkout-err sparse-checkout-err\n> +}\n> +\n> +test_expect_success 'status with options' '\n> +       init_repos &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       run_on_all \"touch README.md\" &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       test_all_match git add README.md &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno\n> +'\n> +\n> +test_expect_success 'add, commit, checkout' '\n> +       init_repos &&\n> +\n> +       write_script edit-contents <<-\\EOF &&\n> +       echo text >>README.md\n> +       EOF\n> +       run_on_all \"../edit-contents\" &&\n> +\n> +       test_all_match git add README.md &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Add README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout - &&\n> +\n> +       run_on_all \"../edit-contents\" &&\n> +\n> +       test_all_match git add -A &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Extend README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout -\n> +'\n\nI was going to add comments here, but I noticed the next test had the\nsame description and looked very similar, so I'll defer the\ncomments...\n\n> +\n> +test_expect_success 'add, commit, checkout' '\n> +       init_repos &&\n> +\n> +       write_script edit-contents <<-\\EOF &&\n> +       echo text >>$1\n> +       EOF\n> +       run_on_all \"../edit-contents README.md\" &&\n> +\n> +       test_all_match git add README.md &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Add README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout - &&\n> +\n> +       run_on_all \"../edit-contents README.md\" &&\n> +\n> +       test_all_match git add -A &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Extend README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout - &&\n\nUp to here, this test is identical to the previous one.  Why repeat it?\n\n> +\n> +       run_on_all \"../edit-contents deep/newfile\" &&\n> +\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git add . &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"add deep/newfile\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout -\n> +'\n\nThink out loud...so you are only adding files that were not previously\ntracked and that would have been part of the sparse cone.  You aren't\ntrying to add files that would be outside the sparse cone, or manually\ncreating files missing from the working tree due to sparseness and\nthen attempting to add them.  (Which is fine, we have to start\nsomewhere with our testing.  Also, I think my testcases didn't look at\nthe case you did, and only covered one of these other two cases.)\n\n> +\n> +test_expect_success 'checkout and reset --hard' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout update-folder1 &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git checkout update-deep &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git checkout -b reset-test &&\n> +       test_all_match git reset --hard deepest &&\n> +       test_all_match git reset --hard update-folder1 &&\n> +       test_all_match git reset --hard update-folder2\n> +'\n> +\n> +test_expect_success 'diff --staged' '\n> +       init_repos &&\n> +\n> +       write_script edit-contents <<-\\EOF &&\n> +       echo text >>README.md\n> +       EOF\n> +       run_on_all \"../edit-contents\" &&\n> +\n> +       test_all_match git diff &&\n> +       test_all_match git diff --staged &&\n> +       test_all_match git add README.md &&\n> +       test_all_match git diff &&\n> +       test_all_match git diff --staged\n> +'\n\nAlso a case where you're operating on a file that matches the sparsity\npatterns (in cone mode, everything in the root directory is included).\n\n> +test_expect_success 'diff with renames' '\n> +       init_repos &&\n> +\n> +       for branch in rename-out-to-out rename-out-to-in rename-in-to-out\n> +       do\n> +               test_all_match git checkout rename-base &&\n> +               test_all_match git checkout $branch -- .&&\n> +               test_all_match git diff --staged &&\n> +               test_all_match git diff --staged --find-renames || return 1\n\nAren't these last two lines the same? (diff.renames defaults to true\never since commit 5404c116aa, \"diff: activate diff.renames by\ndefault\", 2016-02-25)  Are they only different because you have a\ntweaked config that turns off renames by default?\n\nPerhaps the first diff line should have a --no-renames flag.\n\n> +       done\n> +'\n> +\n> +test_expect_success 'log with pathspec outside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git log -- a &&\n> +       test_all_match git log -- folder1/a &&\n> +       test_all_match git log -- folder2/a &&\n> +       test_all_match git log -- deep/a &&\n> +       test_all_match git log -- deep/deeper1/a &&\n> +       test_all_match git log -- deep/deeper1/deepest/a &&\n> +\n> +       test_all_match git checkout update-folder1 &&\n> +       test_all_match git log -- folder1/a\n> +'\n> +\n> +test_expect_success 'blame with pathspec inside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git blame a &&\n> +       test_all_match git blame deep/a &&\n> +       test_all_match git blame deep/deeper1/a &&\n> +       test_all_match git blame deep/deeper1/deepest/a\n> +'\n\nGood check.\n\nOn a side note going back to a piece of the other thread I didn't get\na response to, I'm still curious whether\n    git blame -C -C $PATH_INSIDE_SPARSE_DEFINITION\nshould (optionally?) behave differently in a sparse checkout.  In\nparticular, should it limit its copy detection to other paths also in\nthe sparse checkout, or should it always search all other files within\nthe repository for copied lines?  Searching just within the sparse\ncheckout seems like it could be a really nice performance\noptimization.\n\n> +\n> +# TODO: blame currently does not support blaming files outside of the\n> +# sparse definition. It complains that the file doesn't exist locally.\n\nNice catch.  Yeah, blame tries to check the local working copy for\nchanges, and shows those lines with a changed in commit 0000000000.\nWe should add a check that says that if the file is SKIP_WORKTREE,\nthen we treat it the same as `git blame $PATH HEAD`.\n\n> +test_expect_failure 'blame with pathspec outside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git blame folder1/a &&\n> +       test_all_match git blame folder2/a &&\n> +       test_all_match git blame deep/deeper2/a &&\n> +       test_all_match git blame deep/deeper2/deepest/a\n> +'\n> +\n> +# TODO: reset currently does not behave as expected when in a\n> +# sparse-checkout.\n\nI'm going to go to test this out to see what it does.  It's the first\ntestcase you listed that I didn't know how it worked and couldn't\nfigure it out from your comments.  However it turns out, definitely a\ngood test to have.\n\n> +test_expect_failure 'checkout and reset (mixed)' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout -b reset-test update-deep &&\n> +       test_all_match git reset deepest &&\n> +       test_all_match git reset update-folder1 &&\n> +       test_all_match git reset update-folder2\n> +'\n> +\n> +test_expect_success 'merge' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout -b merge update-deep &&\n> +       test_all_match git merge -m \"folder1\" update-folder1 &&\n> +       test_all_match git rev-parse HEAD^{tree} &&\n> +       test_all_match git merge -m \"folder2\" update-folder2 &&\n> +       test_all_match git rev-parse HEAD^{tree}\n> +'\n> +\n> +test_expect_success 'merge with outside renames' '\n> +       init_repos &&\n> +\n> +       for type in out-to-out out-to-in in-to-out\n> +       do\n> +               test_all_match git reset --hard &&\n> +               test_all_match git checkout -f -b merge-$type update-deep &&\n> +               test_all_match git merge -m \"$type\" rename-$type &&\n> +               test_all_match git rev-parse HEAD^{tree} || return 1\n> +       done\n> +'\n> +\n> +test_expect_success 'clean' '\n> +       init_repos &&\n> +\n> +       echo bogus >>.gitignore &&\n> +       run_on_all cp ../.gitignore . &&\n> +       test_all_match git add .gitignore &&\n> +       test_all_match git commit -m ignore-bogus-files &&\n> +\n> +       run_on_sparse mkdir folder1 &&\n> +       run_on_all touch folder1/bogus &&\n> +\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git clean -f &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git clean -xf &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git clean -xdf &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_path_is_dir sparse-checkout/folder1\n> +'\n> +\n> +test_done\n> --\n\nI made lots of comments, but overall these tests look good to me other\nthan just one question about test duplication and another about using\n--no-renames for diff when rename detection isn't wanted.\n"},{"id":"414848","messageId":"CABPp-BE7wg9YBvFidrQaB3EP+zNb3u9W9_VRhWFdsS85vuYP5A@mail.gmail.com","threadId":"55018","inReplyTo":"f790db9c-0c6d-3661-93d9-0339e22c12ff@gmail.com","subject":"Re: [PATCH 4/9] repository: add repo reference to index_state","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-20T19:50:10Z","receivedAt":"2021-01-20T19:51:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 20, 2021 at 11:16 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 1/20/2021 12:46 PM, Elijah Newren wrote:\n> > On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> >>\n> >> From: Derrick Stolee <dstolee@microsoft.com>\n> >>\n> >> It will be helpful to add behavior to index opertations that might\n> >\n> > s/opertations/operations/\n>\n> Thanks.\n>\n> >> trigger an object lookup. Since each index belongs to a specific\n> >> repository, add a 'repo' pointer to struct index_state that allows\n> >> access to this repository.\n> >>\n> >> This will prevent future changes from needing to pass an additional\n> >> 'struct repository *repo' parameter and instead rely only on the 'struct\n> >> index_state *istate' parameter.\n> >>\n> >> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> >> ---\n> >>  cache.h      | 1 +\n> >>  repository.c | 4 ++++\n> >>  2 files changed, 5 insertions(+)\n> >>\n> >> diff --git a/cache.h b/cache.h\n> >> index 71097657489..f9c7a603841 100644\n> >> --- a/cache.h\n> >> +++ b/cache.h\n> >> @@ -328,6 +328,7 @@ struct index_state {\n> >>         struct ewah_bitmap *fsmonitor_dirty;\n> >>         struct mem_pool *ce_mem_pool;\n> >>         struct progress *progress;\n> >> +       struct repository *repo;\n> >>  };\n> >>\n> >>  /* Name hashing */\n> >> diff --git a/repository.c b/repository.c\n> >> index a4174ddb062..67a4c1da2d9 100644\n> >> --- a/repository.c\n> >> +++ b/repository.c\n> >> @@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)\n> >>         if (!repo->index)\n> >>                 repo->index = xcalloc(1, sizeof(*repo->index));\n> >>\n> >> +       /* Complete the double-reference */\n> >> +       if (!repo->index->repo)\n> >> +               repo->index->repo = repo;\n> >> +\n> >>         return read_index_from(repo->index, repo->index_file, repo->gitdir);\n> >>  }\n> >>\n> >> --\n> >> gitgitgadget\n> >\n> > Since we have repo->index and we have index->repo, which are intended\n> > to be circular...what if they aren't?  Do we want or need to add\n> > assertions anywhere that repo == repo->index->repo or that index ==\n> > index->repo->index ?\n>\n> Here, we are pairing them together and the loop is complete. I don't\n> view that as a permanent thing. This only initializes istate->repo\n> when we are parsing an index from a file, but not when we create one\n> in memory.\n>\n> I imagine it will be likely in some cases to have multiple index_state\n> instances for a single repository. However, having the pointer \"this\n> index belongs to this repository\" seems helpful (to me).\n>\n> > My initial implementations of --remerge-diff[1] played around with\n> > creating a second repo, with a different primary object store but\n> > everything else the same.  The index for the two repository objects\n> > was thus the same, and thus clearly would have violated this assumed\n> > invariant for one of the two repos.  I discarded that initial\n> > implementation (which I didn't quite have working) because I\n> > discovered tmp-objdir.h and was able to add some\n> > tmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions\n> > that merely altered the existing repo's primary object store, but I'm\n> > curious if there might be other cases of folks doing stuff that might\n> > have weird failures with this new invariant.\n>\n> This is an interesting concept, and definitely violates my expectations\n> that an index belongs to only one repository. I'd need to know more\n> about why this was a good design decision before being convinced that\n> the relationship should not be many-to-one (index-to-repo).\n\nI'm not sure what I did was a good design decision; I was kind of\nexploring and trying to figure things out.  In retrospect, I think it\nwas probably a bad idea.  But we have various guard rails in the form\nof BUG() calls and such when basic assumptions are violated, and here\nit seems that you are now making a new basic assumption that an index\nbelongs to only one repository.  (Even if all current callers happen\nto satisfy that assumption, it's not clear to me that git previously\ncared if this condition were satisfied or not).  Hence my question\nabout safety checks.\n\n> > It's entirely possible that --remerge-diff was just so different, and\n> > I was so unfamiliar with repo objects (and still kind of am) that I\n> > was just doing weird stuff no one has done before, so perhaps no\n> > additional checks are needed -- I'm just throwing my gut question out\n> > there as food for thought.\n> >\n> > [1] I have not yet submitted `--remerge-diff` to the list; you haven't\n> > missed anything.  I'm waiting for merge-ort to be submitted, reviewed,\n> > and merged first.  It's the remerge-diff branch in my fork on GitHub\n> > if anyone is curious, though.\n>\n> I'm interested in what others might say about this idea. I'd be able\n> to do most of what I want to do without this patch, but it just gets\n> a lot messier. (istate->repo is used in the very next patch in a way\n> that would be less clean without it.)\n\nI'm less concerned with your patch as-is (I think your assumption\nseems reasonable and I'm fine with labelling my former unsubmitted\npatches as erroneous), and more wondering whether others in the future\nwill accidentally violate assumptions your patch starts encoding...and\nwhether we can or should do anything about it.  If there's a simple\nplace we can add a check for such an error, then it probably makes\nsense to add one.  If there isn't...then at least we considered it?\n"},{"id":"414897","messageId":"bd81bbda-152d-6230-bfc5-f4aa60d0fe9e@gmail.com","threadId":"55018","inReplyTo":"CABPp-BH=E4bR2K21e-p+ABTG02feDF7YNvrOKhsMW5Dtet6y7Q@mail.gmail.com","subject":"Re: [PATCH 9/9] t1092: test interesting sparse-checkout scenarios","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-21T11:59:18Z","receivedAt":"2021-01-21T12:00:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/20/2021 2:40 PM, Elijah Newren wrote:\n> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> These also document some behaviors that differ from a full checkout, and\n>> possibly in a way that is not intended.\n> \n> I'm in favor.  I should turn some of my noted weird behaviors from [1]\n> into testcases as well.\n> \n> [1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/\n\nThat would be helpful, and adding them here would be helpful.\n\nFor the cases where things don't work correctly, it is good\nto add \"test_expect_failure\" cases as a TODO list for the\nfeature space.\n\n>> +\n>> +test_expect_success 'add, commit, checkout' '\n>> +       init_repos &&\n>> +\n>> +       write_script edit-contents <<-\\EOF &&\n>> +       echo text >>$1\n>> +       EOF\n>> +       run_on_all \"../edit-contents README.md\" &&\n>> +\n>> +       test_all_match git add README.md &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +       test_all_match git commit -m \"Add README.md\" &&\n>> +\n>> +       test_all_match git checkout HEAD~1 &&\n>> +       test_all_match git checkout - &&\n>> +\n>> +       run_on_all \"../edit-contents README.md\" &&\n>> +\n>> +       test_all_match git add -A &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +       test_all_match git commit -m \"Extend README.md\" &&\n>> +\n>> +       test_all_match git checkout HEAD~1 &&\n>> +       test_all_match git checkout - &&\n> \n> Up to here, this test is identical to the previous one.  Why repeat it?\n\nUnintentional. I cleaned this up in a later commit of my\nprototype, but forgot to delete that back in this patch.\nWill fix.\n\n>> +\n>> +       run_on_all \"../edit-contents deep/newfile\" &&\n>> +\n>> +       test_all_match git status --porcelain=v2 -uno &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +       test_all_match git add . &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +       test_all_match git commit -m \"add deep/newfile\" &&\n>> +\n>> +       test_all_match git checkout HEAD~1 &&\n>> +       test_all_match git checkout -\n>> +'\n> \n> Think out loud...so you are only adding files that were not previously\n> tracked and that would have been part of the sparse cone.  You aren't\n> trying to add files that would be outside the sparse cone, or manually\n> creating files missing from the working tree due to sparseness and\n> then attempting to add them.  (Which is fine, we have to start\n> somewhere with our testing.  Also, I think my testcases didn't look at\n> the case you did, and only covered one of these other two cases.)\n\nYes, these tests are currently focusing on the \"happy\" cases\nof what is happening within the sparse cone. I plan to expand\nto the more complicated cases later, as I start implementing\nthem correctly with the sparse-index. However, it would be fine\nto have the tests here earlier. Extra documentation of the\nexpected behavior (or how the current implementation is not\ndesirable) would be good.\n\n>> +\n>> +test_expect_success 'checkout and reset --hard' '\n>> +       init_repos &&\n>> +\n>> +       test_all_match git checkout update-folder1 &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +\n>> +       test_all_match git checkout update-deep &&\n>> +       test_all_match git status --porcelain=v2 &&\n>> +\n>> +       test_all_match git checkout -b reset-test &&\n>> +       test_all_match git reset --hard deepest &&\n>> +       test_all_match git reset --hard update-folder1 &&\n>> +       test_all_match git reset --hard update-folder2\n>> +'\n>> +\n>> +test_expect_success 'diff --staged' '\n>> +       init_repos &&\n>> +\n>> +       write_script edit-contents <<-\\EOF &&\n>> +       echo text >>README.md\n>> +       EOF\n>> +       run_on_all \"../edit-contents\" &&\n>> +\n>> +       test_all_match git diff &&\n>> +       test_all_match git diff --staged &&\n>> +       test_all_match git add README.md &&\n>> +       test_all_match git diff &&\n>> +       test_all_match git diff --staged\n>> +'\n> \n> Also a case where you're operating on a file that matches the sparsity\n> patterns (in cone mode, everything in the root directory is included).\n> \n>> +test_expect_success 'diff with renames' '\n>> +       init_repos &&\n>> +\n>> +       for branch in rename-out-to-out rename-out-to-in rename-in-to-out\n>> +       do\n>> +               test_all_match git checkout rename-base &&\n>> +               test_all_match git checkout $branch -- .&&\n>> +               test_all_match git diff --staged &&\n>> +               test_all_match git diff --staged --find-renames || return 1\n> \n> Aren't these last two lines the same? (diff.renames defaults to true\n> ever since commit 5404c116aa, \"diff: activate diff.renames by\n> default\", 2016-02-25)  Are they only different because you have a\n> tweaked config that turns off renames by default?\n> \n> Perhaps the first diff line should have a --no-renames flag.\n\nThanks! Will do.\n\n>> +       done\n>> +'\n>> +\n>> +test_expect_success 'log with pathspec outside sparse definition' '\n>> +       init_repos &&\n>> +\n>> +       test_all_match git log -- a &&\n>> +       test_all_match git log -- folder1/a &&\n>> +       test_all_match git log -- folder2/a &&\n>> +       test_all_match git log -- deep/a &&\n>> +       test_all_match git log -- deep/deeper1/a &&\n>> +       test_all_match git log -- deep/deeper1/deepest/a &&\n>> +\n>> +       test_all_match git checkout update-folder1 &&\n>> +       test_all_match git log -- folder1/a\n>> +'\n>> +\n>> +test_expect_success 'blame with pathspec inside sparse definition' '\n>> +       init_repos &&\n>> +\n>> +       test_all_match git blame a &&\n>> +       test_all_match git blame deep/a &&\n>> +       test_all_match git blame deep/deeper1/a &&\n>> +       test_all_match git blame deep/deeper1/deepest/a\n>> +'\n> \n> Good check.\n> \n> On a side note going back to a piece of the other thread I didn't get\n> a response to, I'm still curious whether\n>     git blame -C -C $PATH_INSIDE_SPARSE_DEFINITION\n> should (optionally?) behave differently in a sparse checkout.  In\n> particular, should it limit its copy detection to other paths also in\n> the sparse checkout, or should it always search all other files within\n> the repository for copied lines?  Searching just within the sparse\n> checkout seems like it could be a really nice performance\n> optimization.\n\nAll of the \"find movements or copies\" logic could benefit from a\n\"universal\" option to restrict to the sparse-checkout definition.\n\n>> +\n>> +# TODO: blame currently does not support blaming files outside of the\n>> +# sparse definition. It complains that the file doesn't exist locally.\n> \n> Nice catch.  Yeah, blame tries to check the local working copy for\n> changes, and shows those lines with a changed in commit 0000000000.\n> We should add a check that says that if the file is SKIP_WORKTREE,\n> then we treat it the same as `git blame $PATH HEAD`.\n\nRight. If it's not in the working directory, then we should\ninterpret that as HEAD.\n\n>> +test_expect_failure 'blame with pathspec outside sparse definition' '\n>> +       init_repos &&\n>> +\n>> +       test_all_match git blame folder1/a &&\n>> +       test_all_match git blame folder2/a &&\n>> +       test_all_match git blame deep/deeper2/a &&\n>> +       test_all_match git blame deep/deeper2/deepest/a\n>> +'\n>> +\n>> +# TODO: reset currently does not behave as expected when in a\n>> +# sparse-checkout.\n> \n> I'm going to go to test this out to see what it does.  It's the first\n> testcase you listed that I didn't know how it worked and couldn't\n> figure it out from your comments.  However it turns out, definitely a\n> good test to have.\n>\n>> +test_expect_failure 'checkout and reset (mixed)' '\n>> +       init_repos &&\n>> +\n>> +       test_all_match git checkout -b reset-test update-deep &&\n>> +       test_all_match git reset deepest &&\n>> +       test_all_match git reset update-folder1 &&\n>> +       test_all_match git reset update-folder2\n>> +'\n\nHere are the failures:\n\nafter \"git checkout -b reset-test update-deep\"\n\n--- full-checkout-out   2021-01-21 11:51:30.059713246 +0000\n+++ sparse-checkout-out 2021-01-21 11:51:30.063713235 +0000\n@@ -1,5 +1,5 @@\n Unstaged changes after reset:\n M      deep/a\n M      deep/deeper1/deepest/a\n-M      folder1/a\n-M      folder2/a\n+D      folder1/a\n+D      folder2/a\n\nIf we comment out that line, the failures for\n\"git reset update-folder1\" is:\n\n--- full-checkout-out   2021-01-21 11:52:36.203509990 +0000\n+++ sparse-checkout-out 2021-01-21 11:52:36.207509976 +0000\n@@ -1,2 +1,2 @@\n Unstaged changes after reset:\n-M      folder1/a\n+D      folder1/a\n\nIf we comment out that line, then the failure for\n\"git reset update-folder2\" is:\n\n--- full-checkout-out   2021-01-21 11:53:55.199197608 +0000\n+++ sparse-checkout-out 2021-01-21 11:53:55.207197573 +0000\n@@ -1,3 +1,3 @@\n Unstaged changes after reset:\n-M      folder1/a\n-M      folder2/a\n+D      folder1/a\n+D      folder2/a\n\nOddly, when I merge this into our branch in microsoft/git,\nthese failures disappear. There is something in those commits\nthat resolve this particular case. I hope to figure that out\nsometime.\n\nThanks,\n-Stolee\n"},{"id":"414898","messageId":"CAPx1GveERs+g6zymyc8AbVudRHz24=N-1WZg+1xj5j1z0prWyg@mail.gmail.com","threadId":"55018","inReplyTo":"838922de2e9756c00f4a159b2b8722ae4d28b011.1611161639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/9] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2021-01-21T12:53:15Z","receivedAt":"2021-01-21T12:54:30Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Jan 20, 2021 at 8:58 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The index has an fsmonitor_dirty bitmap that records which index entries\n> are \"dirty\" based on the response from the FSMonitor. If this bitmap\n> ever grows larger than the index, then there was an error in how it was\n> constructed, and it was probably a developer's bug.\n\nCurious: some of the tests were >=, some were > (not >=).  Now\nthat they're shared in a function they are all \">\".\n\nIt's pretty clear that for size-based ones, greater-than is the\nright test, but for position ones, isn't it still greater-or-equal?  So\nperhaps the calls that pass an actual position should add 1...\n\nChris\n"},{"id":"414911","messageId":"29dc3650-e23d-e283-ae82-69637ae6605e@gmail.com","threadId":"55018","inReplyTo":"CAPx1GveERs+g6zymyc8AbVudRHz24=N-1WZg+1xj5j1z0prWyg@mail.gmail.com","subject":"Re: [PATCH 3/9] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-21T15:56:32Z","receivedAt":"2021-01-21T16:49:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/21/2021 7:53 AM, Chris Torek wrote:\n> On Wed, Jan 20, 2021 at 8:58 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> The index has an fsmonitor_dirty bitmap that records which index entries\n>> are \"dirty\" based on the response from the FSMonitor. If this bitmap\n>> ever grows larger than the index, then there was an error in how it was\n>> constructed, and it was probably a developer's bug.\n> \n> Curious: some of the tests were >=, some were > (not >=).  Now\n> that they're shared in a function they are all \">\".\n> \n> It's pretty clear that for size-based ones, greater-than is the\n> right test, but for position ones, isn't it still greater-or-equal?  So\n> perhaps the calls that pass an actual position should add 1...\n\nThat's a good point. I should pass \"pos + 1\" in the appropriate\nplaces.\n\nThanks,\n-Stolee\n"},{"id":"414958","messageId":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.git.1611161639.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] More index cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:51Z","receivedAt":"2021-01-22T13:05:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is based on ds/cache-tree-basics.\n\nHere are a few more cleanups that are vaguely related to the index. I\ndiscovered these while preparing my sparse-index RFC that I intend to send\nearly next week.\n\nThe biggest patch is the final one, which creates a test script for\ncomparing sparse-checkouts to full checkouts. There are some commands that\ndo not behave similarly. This script will be the backbone of my testing\nstrategy for the sparse-index by adding a new mode to compare\nsparse-checkouts with the two index types (full and sparse).\n\n\nUPDATES IN V2\n=============\n\n * Fixed duplicated test in t1092.\n\n * Changed the implementation of 'test_region' after I discovered the\n   negation doesn't work correctly. (I updated the test to use what was in\n   t0500-progress-display.sh at the last minute before v1, but that\n   implementation was wrong.) The use of it in t0500-progress-display.sh was\n   incorrect, as well.\n\n * Updated commit messages to be more informative and have fewer typos.\n\n * I dropped the patch that placed the sparse-checkout patterns in struct\n   index_state. I'll re-introduce that in time for the actual use of the\n   member.\n\nThanks, -Stolee\n\nDerrick Stolee (8):\n  cache-tree: clean up cache_tree_update()\n  cache-tree: extract subtree_pos()\n  fsmonitor: de-duplicate BUG()s around dirty bits\n  repository: add repo reference to index_state\n  name-hash: use trace2 regions for init\n  sparse-checkout: load sparse-checkout patterns\n  test-lib: test_region looks for trace2 regions\n  t1092: test interesting sparse-checkout scenarios\n\n builtin/sparse-checkout.c                |   5 -\n cache-tree.c                             |  20 +-\n cache-tree.h                             |   2 +\n cache.h                                  |   1 +\n dir.c                                    |  17 ++\n dir.h                                    |   2 +\n fsmonitor.c                              |  27 +-\n name-hash.c                              |   3 +\n repository.c                             |   4 +\n t/t0500-progress-display.sh              |   3 +-\n t/t1092-sparse-checkout-compatibility.sh | 298 +++++++++++++++++++++++\n t/test-lib-functions.sh                  |  40 +++\n unpack-trees.c                           |   6 +-\n 13 files changed, 394 insertions(+), 34 deletions(-)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\n\nbase-commit: a4b6d202caad83c6dc29abe9b17e53a1b3fb54a0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-839%2Fderrickstolee%2Fmore-index-cleanups-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-839/derrickstolee/more-index-cleanups-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/839\n\nRange-diff vs v1:\n\n  1:  0bccfd34ae5 !  1:  f9dccaed0ac cache-tree: clean up cache_tree_update()\n     @@ Commit message\n          cache-tree: clean up cache_tree_update()\n      \n          Make the method safer by allocating a cache_tree member for the given\n     -    index_state if it is not already present.\n     +    index_state if it is not already present. This is preferrable to a\n     +    BUG() statement or returning with an error because future callers will\n     +    want to populate an empty cache-tree using this method.\n      \n          Also drop local variables that are used exactly once and can be found\n          directly from the 'istate' parameter.\n  2:  a6f2406a795 =  2:  84323e04d08 cache-tree: extract subtree_pos()\n  3:  838922de2e9 =  3:  31095f9aa0e fsmonitor: de-duplicate BUG()s around dirty bits\n  4:  d4ff0468fc0 !  4:  a0d89d7a973 repository: add repo reference to index_state\n     @@ Metadata\n       ## Commit message ##\n          repository: add repo reference to index_state\n      \n     -    It will be helpful to add behavior to index opertations that might\n     +    It will be helpful to add behavior to index operations that might\n          trigger an object lookup. Since each index belongs to a specific\n          repository, add a 'repo' pointer to struct index_state that allows\n          access to this repository.\n  5:  3ba4b35f09c =  5:  bc092f5c703 name-hash: use trace2 regions for init\n  6:  64358ec7ea2 =  6:  04d1daf7222 sparse-checkout: load sparse-checkout patterns\n  7:  91344f5108c <  -:  ----------- sparse-checkout: hold pattern list in index\n  8:  8326a9b5320 !  7:  8832ce84623 test-lib: test_region looks for trace2 regions\n     @@ Commit message\n          entered and left in a given trace2 event log.\n      \n          There is one existing test (t0500-progress-display.sh) that performs\n     -    this check already, so use the helper function instead. More uses will\n     -    be added in a later change.\n     +    this check already, so use the helper function instead. Note that this\n     +    changes the expectations slightly. The old test (incorrectly) used two\n     +    patterns for the 'grep' invocation, but this performs an OR of the\n     +    patterns, not an AND. This means that as long as one region_enter event\n     +    was logged, the test would succeed, even if it was not due to the\n     +    progress category.\n     +\n     +    More uses will be added in a later change.\n      \n          t6423-merge-rename-directories.sh also greps for region_enter lines, but\n          it verifies the number of such lines, which is not the same as an\n     @@ t/t0500-progress-display.sh: test_expect_success 'progress generates traces' '\n       \t# t0212/parse_events.perl intentionally omits regions and data.\n      -\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n      -\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n     -+\ttest_region category progress trace.event &&\n     ++\ttest_region progress \"Working hard\" trace.event &&\n       \tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n       \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n       '\n     @@ t/test-lib-functions.sh: test_subcommand () {\n      +\t\tshift\n      +\tfi\n      +\n     -+\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n     ++\tgrep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n      +\texitcode=$?\n      +\n      +\tif test $exitcode != $expect_exit\n     @@ t/test-lib-functions.sh: test_subcommand () {\n      +\t\treturn 1\n      +\tfi\n      +\n     -+\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n     ++\tgrep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n      +\texitcode=$?\n      +\n      +\tif test $exitcode != $expect_exit\n  9:  555e210dc03 !  8:  984458007ed t1092: test interesting sparse-checkout scenarios\n     @@ Commit message\n          These also document some behaviors that differ from a full checkout, and\n          possibly in a way that is not intended.\n      \n     +    The test is designed to be run with \"--run=1,X\" where 'X' is an\n     +    interesting test case. Each test uses 'init_repos' to reset the full and\n     +    sparse copies of the initial-repo that is created by the first test\n     +    case. This also makes it possible to have test cases leave the working\n     +    directory or index in unusual states without disturbing later cases.\n     +\n          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n      \n       ## t/t1092-sparse-checkout-compatibility.sh (new) ##\n     @@ t/t1092-sparse-checkout-compatibility.sh (new)\n      +\tinit_repos &&\n      +\n      +\twrite_script edit-contents <<-\\EOF &&\n     -+\techo text >>README.md\n     -+\tEOF\n     -+\trun_on_all \"../edit-contents\" &&\n     -+\n     -+\ttest_all_match git add README.md &&\n     -+\ttest_all_match git status --porcelain=v2 &&\n     -+\ttest_all_match git commit -m \"Add README.md\" &&\n     -+\n     -+\ttest_all_match git checkout HEAD~1 &&\n     -+\ttest_all_match git checkout - &&\n     -+\n     -+\trun_on_all \"../edit-contents\" &&\n     -+\n     -+\ttest_all_match git add -A &&\n     -+\ttest_all_match git status --porcelain=v2 &&\n     -+\ttest_all_match git commit -m \"Extend README.md\" &&\n     -+\n     -+\ttest_all_match git checkout HEAD~1 &&\n     -+\ttest_all_match git checkout -\n     -+'\n     -+\n     -+test_expect_success 'add, commit, checkout' '\n     -+\tinit_repos &&\n     -+\n     -+\twrite_script edit-contents <<-\\EOF &&\n      +\techo text >>$1\n      +\tEOF\n      +\trun_on_all \"../edit-contents README.md\" &&\n     @@ t/t1092-sparse-checkout-compatibility.sh (new)\n      +\tdo\n      +\t\ttest_all_match git checkout rename-base &&\n      +\t\ttest_all_match git checkout $branch -- .&&\n     -+\t\ttest_all_match git diff --staged &&\n     ++\t\ttest_all_match git diff --staged --no-renames &&\n      +\t\ttest_all_match git diff --staged --find-renames || return 1\n      +\tdone\n      +'\n\n-- \ngitgitgadget\n"},{"id":"414959","messageId":"8832ce84623e9c74a88b14a05b1c303ed8aa809b.1611320640.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 7/8] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:58Z","receivedAt":"2021-01-22T13:06:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMost test cases can verify Git's behavior using input/output\nexpectations or changes to the .git directory. However, sometimes we\nwant to check that Git did or did not run a certain section of code.\nThis is particularly important for performance-only features that we\nwant to ensure have been enabled in certain cases.\n\nAdd a new 'test_region' function that checks if a trace2 region was\nentered and left in a given trace2 event log.\n\nThere is one existing test (t0500-progress-display.sh) that performs\nthis check already, so use the helper function instead. Note that this\nchanges the expectations slightly. The old test (incorrectly) used two\npatterns for the 'grep' invocation, but this performs an OR of the\npatterns, not an AND. This means that as long as one region_enter event\nwas logged, the test would succeed, even if it was not due to the\nprogress category.\n\nMore uses will be added in a later change.\n\nt6423-merge-rename-directories.sh also greps for region_enter lines, but\nit verifies the number of such lines, which is not the same as an\nexistence check.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t0500-progress-display.sh |  3 +--\n t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 1ed1df351cb..84cce345e7d 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '\n \t\t\"Working hard\" <in 2>stderr &&\n \n \t# t0212/parse_events.perl intentionally omits regions and data.\n-\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n-\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\ttest_region progress \"Working hard\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a9..1170f7108ac 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1655,3 +1655,43 @@ test_subcommand () {\n \t\tgrep \"\\[$expr\\]\"\n \tfi\n }\n+\n+# Check that the given command was invoked as part of the\n+# trace2-format trace on stdin.\n+#\n+#\ttest_region [!] <category> <label> git <command> <args>...\n+#\n+# For example, to look for trace2_region_enter(\"index\", \"do_read_index\", repo)\n+# in an invocation of \"git checkout HEAD~1\", run\n+#\n+#\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+#\t\tgit checkout HEAD~1 &&\n+#\ttest_region index do_read_index <trace.txt\n+#\n+# If the first parameter passed is !, this instead checks that\n+# the given region was not entered.\n+#\n+test_region () {\n+\tlocal expect_exit=0\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\texpect_exit=1\n+\t\tshift\n+\tfi\n+\n+\tgrep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tgrep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+}\n-- \ngitgitgadget\n\n"},{"id":"414960","messageId":"984458007edb9ac93630490cea1b0863fb584310.1611320640.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 8/8] t1092: test interesting sparse-checkout scenarios","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:59Z","receivedAt":"2021-01-22T13:06:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThese also document some behaviors that differ from a full checkout, and\npossibly in a way that is not intended.\n\nThe test is designed to be run with \"--run=1,X\" where 'X' is an\ninteresting test case. Each test uses 'init_repos' to reset the full and\nsparse copies of the initial-repo that is created by the first test\ncase. This also makes it possible to have test cases leave the working\ndirectory or index in unusual states without disturbing later cases.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t1092-sparse-checkout-compatibility.sh | 298 +++++++++++++++++++++++\n 1 file changed, 298 insertions(+)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nnew file mode 100755\nindex 00000000000..141cbe822b6\n--- /dev/null\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -0,0 +1,298 @@\n+#!/bin/sh\n+\n+test_description='compare full workdir to sparse workdir'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init initial-repo &&\n+\t(\n+\t\tcd initial-repo &&\n+\t\techo a >a &&\n+\t\techo \"after deep\" >e &&\n+\t\techo \"after folder1\" >g &&\n+\t\tmkdir folder1 folder2 deep x &&\n+\t\tmkdir deep/deeper1 deep/deeper2 &&\n+\t\tmkdir deep/deeper1/deepest &&\n+\t\techo \"after deeper1\" >deep/e &&\n+\t\techo \"after deepest\" >deep/deeper1/e &&\n+\t\tcp a folder1 &&\n+\t\tcp a folder2 &&\n+\t\tcp a deep &&\n+\t\tcp a deep/deeper1 &&\n+\t\tcp a deep/deeper2 &&\n+\t\tcp a deep/deeper1/deepest &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit checkout -b base &&\n+\t\tfor dir in folder1 folder2 deep\n+\t\tdo\n+\t\t\tgit checkout -b update-$dir &&\n+\t\t\techo \"updated $dir\" >$dir/a &&\n+\t\t\tgit commit -a -m \"update $dir\" || return 1\n+\t\tdone &&\n+\n+\t\tgit checkout -b rename-base base &&\n+\t\techo >folder1/larger-content <<-\\EOF &&\n+\t\tmatching\n+\t\tlines\n+\t\thelp\n+\t\tinexact\n+\t\trenames\n+\t\tEOF\n+\t\tcp folder1/larger-content folder2/ &&\n+\t\tcp folder1/larger-content deep/deeper1/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"add interesting rename content\" &&\n+\n+\t\tgit checkout -b rename-out-to-out rename-base &&\n+\t\tmv folder1/a folder2/b &&\n+\t\tmv folder1/larger-content folder2/edited-content &&\n+\t\techo >>folder2/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to folder2/...\" &&\n+\n+\t\tgit checkout -b rename-out-to-in rename-base &&\n+\t\tmv folder1/a deep/deeper1/b &&\n+\t\tmv folder1/larger-content deep/deeper1/edited-content &&\n+\t\techo >>deep/deeper1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to deep/deeper1/...\" &&\n+\n+\t\tgit checkout -b rename-in-to-out rename-base &&\n+\t\tmv deep/deeper1/a folder1/b &&\n+\t\tmv deep/deeper1/larger-content folder1/edited-content &&\n+\t\techo >>folder1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename deep/deeper1/... to folder1/...\" &&\n+\n+\t\tgit checkout -b deepest base &&\n+\t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n+\t\tgit commit -a -m \"update deepest\" &&\n+\n+\t\tgit checkout -f base &&\n+\t\tgit reset --hard\n+\t)\n+'\n+\n+init_repos () {\n+\trm -rf full-checkout sparse-checkout sparse-index &&\n+\n+\t# create repos in initial state\n+\tcp -r initial-repo full-checkout &&\n+\tgit -C full-checkout reset --hard &&\n+\n+\tcp -r initial-repo sparse-checkout &&\n+\tgit -C sparse-checkout reset --hard &&\n+\tgit -C sparse-checkout sparse-checkout init --cone &&\n+\n+\t# initialize sparse-checkout definitions\n+\tgit -C sparse-checkout sparse-checkout set deep\n+}\n+\n+run_on_sparse () {\n+\t(\n+\t\tcd sparse-checkout &&\n+\t\t$* >../sparse-checkout-out 2>../sparse-checkout-err\n+\t)\n+}\n+\n+run_on_all () {\n+\t(\n+\t\tcd full-checkout &&\n+\t\t$* >../full-checkout-out 2>../full-checkout-err\n+\t) &&\n+\trun_on_sparse $*\n+}\n+\n+test_all_match () {\n+\trun_on_all $* &&\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err\n+}\n+\n+test_expect_success 'status with options' '\n+\tinit_repos &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\trun_on_all \"touch README.md\" &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno\n+'\n+\n+test_expect_success 'add, commit, checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Add README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add -A &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Extend README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents deep/newfile\" &&\n+\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git add . &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"add deep/newfile\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout -\n+'\n+\n+test_expect_success 'checkout and reset --hard' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout update-deep &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout -b reset-test &&\n+\ttest_all_match git reset --hard deepest &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\ttest_all_match git reset --hard update-folder2\n+'\n+\n+test_expect_success 'diff --staged' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>README.md\n+\tEOF\n+\trun_on_all \"../edit-contents\" &&\n+\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged\n+'\n+\n+test_expect_success 'diff with renames' '\n+\tinit_repos &&\n+\n+\tfor branch in rename-out-to-out rename-out-to-in rename-in-to-out\n+\tdo\n+\t\ttest_all_match git checkout rename-base &&\n+\t\ttest_all_match git checkout $branch -- .&&\n+\t\ttest_all_match git diff --staged --no-renames &&\n+\t\ttest_all_match git diff --staged --find-renames || return 1\n+\tdone\n+'\n+\n+test_expect_success 'log with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git log -- a &&\n+\ttest_all_match git log -- folder1/a &&\n+\ttest_all_match git log -- folder2/a &&\n+\ttest_all_match git log -- deep/a &&\n+\ttest_all_match git log -- deep/deeper1/a &&\n+\ttest_all_match git log -- deep/deeper1/deepest/a &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git log -- folder1/a\n+'\n+\n+test_expect_success 'blame with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame a &&\n+\ttest_all_match git blame deep/a &&\n+\ttest_all_match git blame deep/deeper1/a &&\n+\ttest_all_match git blame deep/deeper1/deepest/a\n+'\n+\n+# TODO: blame currently does not support blaming files outside of the\n+# sparse definition. It complains that the file doesn't exist locally.\n+test_expect_failure 'blame with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame folder1/a &&\n+\ttest_all_match git blame folder2/a &&\n+\ttest_all_match git blame deep/deeper2/a &&\n+\ttest_all_match git blame deep/deeper2/deepest/a\n+'\n+\n+# TODO: reset currently does not behave as expected when in a\n+# sparse-checkout.\n+test_expect_failure 'checkout and reset (mixed)' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b reset-test update-deep &&\n+\ttest_all_match git reset deepest &&\n+\ttest_all_match git reset update-folder1 &&\n+\ttest_all_match git reset update-folder2\n+'\n+\n+test_expect_success 'merge' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge update-deep &&\n+\ttest_all_match git merge -m \"folder1\" update-folder1 &&\n+\ttest_all_match git rev-parse HEAD^{tree} &&\n+\ttest_all_match git merge -m \"folder2\" update-folder2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n+test_expect_success 'merge with outside renames' '\n+\tinit_repos &&\n+\n+\tfor type in out-to-out out-to-in in-to-out\n+\tdo\n+\t\ttest_all_match git reset --hard &&\n+\t\ttest_all_match git checkout -f -b merge-$type update-deep &&\n+\t\ttest_all_match git merge -m \"$type\" rename-$type &&\n+\t\ttest_all_match git rev-parse HEAD^{tree} || return 1\n+\tdone\n+'\n+\n+test_expect_success 'clean' '\n+\tinit_repos &&\n+\n+\techo bogus >>.gitignore &&\n+\trun_on_all cp ../.gitignore . &&\n+\ttest_all_match git add .gitignore &&\n+\ttest_all_match git commit -m ignore-bogus-files &&\n+\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all touch folder1/bogus &&\n+\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git clean -f &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xdf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_path_is_dir sparse-checkout/folder1\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"414961","messageId":"04d1daf7222e16f07f9c1eb92a7ecd020667da3c.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 6/8] sparse-checkout: load sparse-checkout patterns","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:57Z","receivedAt":"2021-01-22T13:06:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nA future feature will want to load the sparse-checkout patterns into a\npattern_list, but the current mechanism to do so is a bit complicated.\nThis is made difficult due to needing to find the sparse-checkout file\nin different ways throughout the codebase.\n\nThe logic implemented in the new get_sparse_checkout_patterns() was\nduplicated in populate_from_existing_patterns() in unpack-trees.c. Use\nthe new method instead, keeping the logic around handling the struct\nunpack_trees_options.\n\nThe callers to get_sparse_checkout_filename() in\nbuiltin/sparse-checkout.c manipulate the sparse-checkout file directly,\nso it is not appropriate to replace logic in that file with\nget_sparse_checkout_patterns().\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/sparse-checkout.c |  5 -----\n dir.c                     | 17 +++++++++++++++++\n dir.h                     |  2 ++\n unpack-trees.c            |  6 +-----\n 4 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex e3140db2a0a..2306a9ad98e 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {\n \tNULL\n };\n \n-static char *get_sparse_checkout_filename(void)\n-{\n-\treturn git_pathdup(\"info/sparse-checkout\");\n-}\n-\n static void write_patterns_to_file(FILE *fp, struct pattern_list *pl)\n {\n \tint i;\ndiff --git a/dir.c b/dir.c\nindex d637461da5c..d153a63bbd1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2998,6 +2998,23 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t}\n }\n \n+char *get_sparse_checkout_filename(void)\n+{\n+\treturn git_pathdup(\"info/sparse-checkout\");\n+}\n+\n+int get_sparse_checkout_patterns(struct pattern_list *pl)\n+{\n+\tint res;\n+\tchar *sparse_filename = get_sparse_checkout_filename();\n+\n+\tpl->use_cone_patterns = core_sparse_checkout_cone;\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL);\n+\n+\tfree(sparse_filename);\n+\treturn res;\n+}\n+\n int remove_path(const char *name)\n {\n \tchar *slash;\ndiff --git a/dir.h b/dir.h\nindex a3c40dec516..facfae47402 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -448,6 +448,8 @@ int is_empty_dir(const char *dir);\n \n void setup_standard_excludes(struct dir_struct *dir);\n \n+char *get_sparse_checkout_filename(void);\n+int get_sparse_checkout_patterns(struct pattern_list *pl);\n \n /* Constants for remove_dir_recursively: */\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex af6e9b9c2fd..837b8bb42fb 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1549,14 +1549,10 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n static void populate_from_existing_patterns(struct unpack_trees_options *o,\n \t\t\t\t\t    struct pattern_list *pl)\n {\n-\tchar *sparse = git_pathdup(\"info/sparse-checkout\");\n-\n-\tpl->use_cone_patterns = core_sparse_checkout_cone;\n-\tif (add_patterns_from_file_to_list(sparse, \"\", 0, pl, NULL) < 0)\n+\tif (get_sparse_checkout_patterns(pl) < 0)\n \t\to->skip_sparse_checkout = 1;\n \telse\n \t\to->pl = pl;\n-\tfree(sparse);\n }\n \n \n-- \ngitgitgadget\n\n"},{"id":"414962","messageId":"a0d89d7a9731adeee4e2a432843b2f4a9c232dc4.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 4/8] repository: add repo reference to index_state","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:55Z","receivedAt":"2021-01-22T13:08:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nIt will be helpful to add behavior to index operations that might\ntrigger an object lookup. Since each index belongs to a specific\nrepository, add a 'repo' pointer to struct index_state that allows\naccess to this repository.\n\nThis will prevent future changes from needing to pass an additional\n'struct repository *repo' parameter and instead rely only on the 'struct\nindex_state *istate' parameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache.h      | 1 +\n repository.c | 4 ++++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 71097657489..f9c7a603841 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -328,6 +328,7 @@ struct index_state {\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\n+\tstruct repository *repo;\n };\n \n /* Name hashing */\ndiff --git a/repository.c b/repository.c\nindex a4174ddb062..67a4c1da2d9 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)\n \tif (!repo->index)\n \t\trepo->index = xcalloc(1, sizeof(*repo->index));\n \n+\t/* Complete the double-reference */\n+\tif (!repo->index->repo)\n+\t\trepo->index->repo = repo;\n+\n \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"414963","messageId":"bc092f5c703fbaead337291cb6f731ec342a89f5.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 5/8] name-hash: use trace2 regions for init","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:56Z","receivedAt":"2021-01-22T13:09:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe lazy_init_name_hash() populates a hashset with all filenames and\nanother with all directories represented in the index. This is run only\nif we need to use the hashsets to check for existence or case-folding\nrenames.\n\nPlace trace2 regions where there is already a performance trace.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n name-hash.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex 5d3c7b12c18..4e03fac9bb1 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -7,6 +7,7 @@\n  */\n #include \"cache.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct dir_entry {\n \tstruct hashmap_entry ent;\n@@ -577,6 +578,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \tif (istate->name_hash_initialized)\n \t\treturn;\n \ttrace_performance_enter();\n+\ttrace2_region_enter(\"index\", \"name-hash-init\", istate->repo);\n \thashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n \thashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);\n \n@@ -597,6 +599,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t}\n \n \tistate->name_hash_initialized = 1;\n+\ttrace2_region_leave(\"index\", \"name-hash-init\", istate->repo);\n \ttrace_performance_leave(\"initialize name hash\");\n }\n \n-- \ngitgitgadget\n\n"},{"id":"414964","messageId":"31095f9aa0ecd29193cc4d612d1953653c04b8ae.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 3/8] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:54Z","receivedAt":"2021-01-22T13:10:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe index has an fsmonitor_dirty bitmap that records which index entries\nare \"dirty\" based on the response from the FSMonitor. If this bitmap\never grows larger than the index, then there was an error in how it was\nconstructed, and it was probably a developer's bug.\n\nThere are several BUG() statements that are very similar, so replace\nthese uses with a simpler assert_index_minimum(). Since there is one\ncaller that uses a custom 'pos' value instead of the bit_size member, we\ncannot simplify it too much. However, the error string is identical in\neach, so this simplifies things.\n\nThe end result is that the code is simpler to read while also preserving\nthese assertions for developers in the FSMonitor space.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n fsmonitor.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ca031c3abb8..52a50a9545a 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -13,14 +13,19 @@\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n+static void assert_index_minimum(struct index_state *istate, size_t pos)\n+{\n+\tif (pos > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n+\t\t    (uintmax_t)pos, istate->cache_nr);\n+}\n+\n static void fsmonitor_ewah_callback(size_t pos, void *is)\n {\n \tstruct index_state *istate = (struct index_state *)is;\n \tstruct cache_entry *ce;\n \n-\tif (pos >= istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n-\t\t    (uintmax_t)pos, istate->cache_nr);\n+\tassert_index_minimum(istate, pos);\n \n \tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n@@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t}\n \n \t\t\t/* Mark all previously saved entries as dirty */\n-\t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\t\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n \t\t\trefresh_fsmonitor(istate);\n-- \ngitgitgadget\n\n"},{"id":"414965","messageId":"84323e04d0885d2bb12329e7bd16582d66989c7f.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 2/8] cache-tree: extract subtree_pos()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:53Z","receivedAt":"2021-01-22T13:10:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThis method will be helpful to use outside of cache-tree.c in a later\nfeature. The implementation is subtle due to subtree_name_cmp() sorting\nby length and then lexicographically.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 6 +++---\n cache-tree.h | 2 ++\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex c1e49901c17..2b130dd5e19 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,\n \treturn memcmp(one, two, onelen);\n }\n \n-static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n {\n \tstruct cache_tree_sub **down = it->down;\n \tint lo, hi;\n@@ -72,7 +72,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,\n \t\t\t\t\t   int create)\n {\n \tstruct cache_tree_sub *down;\n-\tint pos = subtree_pos(it, path, pathlen);\n+\tint pos = cache_tree_subtree_pos(it, path, pathlen);\n \tif (0 <= pos)\n \t\treturn it->down[pos];\n \tif (!create)\n@@ -123,7 +123,7 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)\n \tit->entry_count = -1;\n \tif (!*slash) {\n \t\tint pos;\n-\t\tpos = subtree_pos(it, path, namelen);\n+\t\tpos = cache_tree_subtree_pos(it, path, namelen);\n \t\tif (0 <= pos) {\n \t\t\tcache_tree_free(&it->down[pos]->cache_tree);\n \t\t\tfree(it->down[pos]);\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 639bfa5340e..8efeccebfc9 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -27,6 +27,8 @@ void cache_tree_free(struct cache_tree **);\n void cache_tree_invalidate_path(struct index_state *, const char *);\n struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);\n \n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);\n+\n void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n-- \ngitgitgadget\n\n"},{"id":"414966","messageId":"f9dccaed0acae91b06f5f9ccaf0f9116b65f3f09.1611320639.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v2 1/8] cache-tree: clean up cache_tree_update()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-22T13:03:52Z","receivedAt":"2021-01-22T13:10:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMake the method safer by allocating a cache_tree member for the given\nindex_state if it is not already present. This is preferrable to a\nBUG() statement or returning with an error because future callers will\nwant to populate an empty cache-tree using this method.\n\nAlso drop local variables that are used exactly once and can be found\ndirectly from the 'istate' parameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 3f1a8d4f1b7..c1e49901c17 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n-\tstruct cache_tree *it = istate->cache_tree;\n-\tstruct cache_entry **cache = istate->cache;\n-\tint entries = istate->cache_nr;\n-\tint skip, i = verify_cache(cache, entries, flags);\n+\tint skip, i;\n+\n+\ti = verify_cache(istate->cache, istate->cache_nr, flags);\n \n \tif (i)\n \t\treturn i;\n+\n+\tif (!istate->cache_tree)\n+\t\tistate->cache_tree = cache_tree();\n+\n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\ti = update_one(it, cache, entries, \"\", 0, &skip, flags);\n+\ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n+\t\t       \"\", 0, &skip, flags);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n-- \ngitgitgadget\n\n"},{"id":"414984","messageId":"xmqqlfck948f.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"f9dccaed0acae91b06f5f9ccaf0f9116b65f3f09.1611320639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/8] cache-tree: clean up cache_tree_update()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T19:11:12Z","receivedAt":"2021-01-22T19:12:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Make the method safer by allocating a cache_tree member for the given\n> index_state if it is not already present. This is preferrable to a\n> BUG() statement or returning with an error because future callers will\n> want to populate an empty cache-tree using this method.\n\nHow do the current callers prepare the istate to be passed to this\nfunction?  The implications of this question obviously are:\n\n - why is it unreasonable for future callers to follow suit?  or\n\n - now the callers are no longer responsible to give an empty\n   cache_tree to istate before calling this function, shouldn't\n   existing callers lose code that is no longer needed?\n\nI would imagine that the first is easily answered with \"but that\nwould be more code on the callers' side\", but then the second one\nbecomes even more relevant, no?\n\n> Also drop local variables that are used exactly once and can be found\n> directly from the 'istate' parameter.\n\nIt actuall is used twice, no?\n\nI do find it an improvement because it makes it clearer that\nistate->{cache,cache_nr} comes in pairs.\n\nI wonder if verify_cache() wants to take istate as a parameter,\nreplacing the first two?  update_one() shifts where it starts\nworking in the array and reduces the number of entries as it digs\ndeeper, so it still has to keep taking the (base, nr) pair (iow, its\nsecond and third parameters cannot be replaced with an istate).\n\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  cache-tree.c | 14 +++++++++-----\n>  1 file changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 3f1a8d4f1b7..c1e49901c17 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,\n>  \n>  int cache_tree_update(struct index_state *istate, int flags)\n>  {\n> -\tstruct cache_tree *it = istate->cache_tree;\n> -\tstruct cache_entry **cache = istate->cache;\n> -\tint entries = istate->cache_nr;\n> -\tint skip, i = verify_cache(cache, entries, flags);\n> +\tint skip, i;\n> +\n> +\ti = verify_cache(istate->cache, istate->cache_nr, flags);\n>  \n>  \tif (i)\n>  \t\treturn i;\n> +\n> +\tif (!istate->cache_tree)\n> +\t\tistate->cache_tree = cache_tree();\n> +\n>  \ttrace_performance_enter();\n>  \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> -\ti = update_one(it, cache, entries, \"\", 0, &skip, flags);\n> +\ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> +\t\t       \"\", 0, &skip, flags);\n>  \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n>  \ttrace_performance_leave(\"cache_tree_update\");\n>  \tif (i < 0)\n"},{"id":"415012","messageId":"xmqqczxw93ok.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"a0d89d7a9731adeee4e2a432843b2f4a9c232dc4.1611320639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/8] repository: add repo reference to index_state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T19:23:07Z","receivedAt":"2021-01-22T22:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> It will be helpful to add behavior to index operations that might\n> trigger an object lookup. Since each index belongs to a specific\n> repository, add a 'repo' pointer to struct index_state that allows\n> access to this repository.\n>\n> This will prevent future changes from needing to pass an additional\n> 'struct repository *repo' parameter and instead rely only on the 'struct\n> index_state *istate' parameter.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n\nI think this makes sense, but shouldn't we insist on these\nbidirectional links to point at each other?  Otherwise we cannot\nsimplify the function signatures safely later.  That is ...\n\n> +\t/* Complete the double-reference */\n> +\tif (!repo->index->repo)\n> +\t\trepo->index->repo = repo;\n> +\n\n\telse if (repo->index->repo != repo)\n\t\tBUG(\"the repo->index instance does not belong to the repo???\");\n\n... a check like this?\n\n>  \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n>  }\n"},{"id":"415015","messageId":"xmqqh7n893vx.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"31095f9aa0ecd29193cc4d612d1953653c04b8ae.1611320639.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/8] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T19:18:42Z","receivedAt":"2021-01-22T22:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The index has an fsmonitor_dirty bitmap that records which index entries\n> are \"dirty\" based on the response from the FSMonitor. If this bitmap\n> ever grows larger than the index, then there was an error in how it was\n> constructed, and it was probably a developer's bug.\n>\n> There are several BUG() statements that are very similar, so replace\n> these uses with a simpler assert_index_minimum(). Since there is one\n> caller that uses a custom 'pos' value instead of the bit_size member, we\n> cannot simplify it too much. However, the error string is identical in\n> each, so this simplifies things.\n\nAlso that single caller with a custom 'pos' used to allow 'pos' to\npoint one beyond the end of istate->cache[] array, but now it is\nforbidden.  If this is a strict bugfix, it probably deserves a\nmention here.\n\n> The end result is that the code is simpler to read while also preserving\n> these assertions for developers in the FSMonitor space.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  fsmonitor.c | 27 +++++++++++++--------------\n>  1 file changed, 13 insertions(+), 14 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index ca031c3abb8..52a50a9545a 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -13,14 +13,19 @@\n>  \n>  struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n>  \n> +static void assert_index_minimum(struct index_state *istate, size_t pos)\n> +{\n> +\tif (pos > istate->cache_nr)\n> +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> +\t\t    (uintmax_t)pos, istate->cache_nr);\n> +}\n> +\n>  static void fsmonitor_ewah_callback(size_t pos, void *is)\n>  {\n>  \tstruct index_state *istate = (struct index_state *)is;\n>  \tstruct cache_entry *ce;\n>  \n> -\tif (pos >= istate->cache_nr)\n> -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n> -\t\t    (uintmax_t)pos, istate->cache_nr);\n> +\tassert_index_minimum(istate, pos);\n>  \n>  \tce = istate->cache[pos];\n>  \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n> @@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n>  \t}\n>  \tistate->fsmonitor_dirty = fsmonitor_dirty;\n>  \n> -\tif (!istate->split_index &&\n> -\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\tif (!istate->split_index)\n> +\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>  \n>  \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n>  \treturn 0;\n> @@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n>  \tuint32_t ewah_size = 0;\n>  \tint fixup = 0;\n>  \n> -\tif (!istate->split_index &&\n> -\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\tif (!istate->split_index)\n> +\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>  \n>  \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n>  \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n> @@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)\n>  \t\t\t}\n>  \n>  \t\t\t/* Mark all previously saved entries as dirty */\n> -\t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> -\t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> -\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> +\t\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n>  \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n>  \n>  \t\t\trefresh_fsmonitor(istate);\n"},{"id":"415016","messageId":"CABPp-BHfVs3Yb-QqeTPcdaa_hSVbqyds6ZQ2kX_wMQmWwAEdvQ@mail.gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/8] More index cleanups","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-22T19:49:57Z","receivedAt":"2021-01-22T22:35:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 22, 2021 at 5:04 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> This is based on ds/cache-tree-basics.\n>\n> Here are a few more cleanups that are vaguely related to the index. I\n> discovered these while preparing my sparse-index RFC that I intend to send\n> early next week.\n>\n> The biggest patch is the final one, which creates a test script for\n> comparing sparse-checkouts to full checkouts. There are some commands that\n> do not behave similarly. This script will be the backbone of my testing\n> strategy for the sparse-index by adding a new mode to compare\n> sparse-checkouts with the two index types (full and sparse).\n>\n>\n> UPDATES IN V2\n> =============\n>\n>  * Fixed duplicated test in t1092.\n>\n>  * Changed the implementation of 'test_region' after I discovered the\n>    negation doesn't work correctly. (I updated the test to use what was in\n>    t0500-progress-display.sh at the last minute before v1, but that\n>    implementation was wrong.) The use of it in t0500-progress-display.sh was\n>    incorrect, as well.\n>\n>  * Updated commit messages to be more informative and have fewer typos.\n>\n>  * I dropped the patch that placed the sparse-checkout patterns in struct\n>    index_state. I'll re-introduce that in time for the actual use of the\n>    member.\n\nYou've addressed all my feedback from v1, but it looks like you missed\nthe pos + 1 changes highlighted by Chris in his review of patch 3.\nOversight?\n"},{"id":"415018","messageId":"xmqq7do492sh.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"8832ce84623e9c74a88b14a05b1c303ed8aa809b.1611320640.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/8] test-lib: test_region looks for trace2 regions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T19:42:22Z","receivedAt":"2021-01-22T22:46:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> ... Note that this\n> changes the expectations slightly. The old test (incorrectly) used two\n> patterns for the 'grep' invocation, but this performs an OR of the\n> patterns, not an AND. This means that as long as one region_enter event\n> was logged, the test would succeed, even if it was not due to the\n> progress category.\n\n>  \t# t0212/parse_events.perl intentionally omits regions and data.\n> -\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> -\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\ttest_region progress \"Working hard\" trace.event &&\n\nSo we do want \"region_enter\" and the \"category\":\"progress\" on the\nsame line in the event file, but as long as \"category\":\"progress\"\nexists, both will pass, regardless of enter/leave.  And ...\n\n> +test_region () {\n> +\tlocal expect_exit=0\n> +\tif test \"$1\" = \"!\"\n> +\tthen\n> +\t\texpect_exit=1\n> +\t\tshift\n> +\tfi\n> +\n> +\tgrep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n\n... this makes sure there is enter/category on a line (and\nleave/category on a line with another check).  Makes sense.\n\nBut...\n\n\ttest_region '!' '\\(unmatching capture)' 'two' 'three'\n\nwould try to use an invalid regexp and cause grep to exit with 2,\nwhich would mean ...\n\n> +\texitcode=$?\n> +\n> +\tif test $exitcode != $expect_exit\n\n... this will not trigger and we return \"success\" (i.e. \"failed as\nexpected\")?\n\n\tClarification.  The point is *NOT* that the grep pattern is\n\tnot robust against funnies in $1 and $2---after all, these\n\tstrings are under our control.  The point is what should\n\thappen when \"grep\" exits with an error when asked to ensure\n\tthat there is no region detected.\n\n> +\tthen\n> +\t\treturn 1\n> +\tfi\n> +\n> +\tgrep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n\nThe same comment on \"what about an error from grep\" applies to this\none.\n\nIt might be easier to read to avoid having to say too many\nbackslash-quoted double quotes:\n\n\tgrep -e\t'\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n\nThis comment applies to the earlier \"grep\", too.\n\n> +\texitcode=$?\n> +\n> +\tif test $exitcode != $expect_exit\n> +\tthen\n> +\t\treturn 1\n> +\tfi\n> +}\n"},{"id":"415074","messageId":"8406512b-3d9f-e899-24fd-8a09c4af3569@gmail.com","threadId":"55018","inReplyTo":"xmqq7do492sh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 7/8] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T18:36:06Z","receivedAt":"2021-01-23T18:37:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/22/2021 2:42 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> +\tgrep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n> \n> ... this makes sure there is enter/category on a line (and\n> leave/category on a line with another check).  Makes sense.\n> \n> But...\n> \n> \ttest_region '!' '\\(unmatching capture)' 'two' 'three'\n> \n> would try to use an invalid regexp and cause grep to exit with 2,\n> which would mean ...\n> \n>> +\texitcode=$?\n>> +\n>> +\tif test $exitcode != $expect_exit\n> \n> ... this will not trigger and we return \"success\" (i.e. \"failed as\n> expected\")?\n\nAm I misunderstanding something here? If exitcode is 2, then this\nwill always trigger and return 1, signaling a failure. That would\npropagate to the parent test and cause the test to fail. That seems\nlike the correct intention, but I'm not 100% confident about that.\n\n> \tClarification.  The point is *NOT* that the grep pattern is\n> \tnot robust against funnies in $1 and $2---after all, these\n> \tstrings are under our control.  The point is what should\n> \thappen when \"grep\" exits with an error when asked to ensure\n> \tthat there is no region detected.\n\nI'll be more robust to these in the next version. We'll expect\nexit code equal to zero or _not_ equal to zero, depending on the\npresence of '!'. This has the downside of returning success for\nbad input strings when '!' is specified.\n\nBasically, the approach I'm taking for v3 is here:\n\n\tif [test $expect_exit = 1] && [test $exitcode = 0]\n\tthen\n\t\treturn 1\n\telif [test $expect_exit = 0] && [test $exitcode != 0]\n\tthen\n\t\treturn 1\n\tfi\n\n>> +\tgrep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n> \n> The same comment on \"what about an error from grep\" applies to this\n> one.\n> \n> It might be easier to read to avoid having to say too many\n> backslash-quoted double quotes:\n> \n> \tgrep -e\t'\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n> \n> This comment applies to the earlier \"grep\", too.\n\nThanks. This does look a bit cleaner.\n\n-Stolee\n"},{"id":"415075","messageId":"a6762eae-6c63-0e15-f5b9-14f69e1f2593@gmail.com","threadId":"55018","inReplyTo":"CABPp-BHfVs3Yb-QqeTPcdaa_hSVbqyds6ZQ2kX_wMQmWwAEdvQ@mail.gmail.com","subject":"Re: [PATCH v2 0/8] More index cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T18:47:55Z","receivedAt":"2021-01-23T18:48:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/22/2021 2:49 PM, Elijah Newren wrote:\n> \n> You've addressed all my feedback from v1, but it looks like you missed\n> the pos + 1 changes highlighted by Chris in his review of patch 3.\n> Oversight?\n \nYes, oversight. Thank you for the reminder.\n\n-Stolee\n"},{"id":"415076","messageId":"xmqqczxv4hdp.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"8406512b-3d9f-e899-24fd-8a09c4af3569@gmail.com","subject":"Re: [PATCH v2 7/8] test-lib: test_region looks for trace2 regions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-23T18:50:42Z","receivedAt":"2021-01-23T18:51:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>>> +\tif test $exitcode != $expect_exit\n>> \n>> ... this will not trigger and we return \"success\" (i.e. \"failed as\n>> expected\")?\n>\n> Am I misunderstanding something here? If exitcode is 2, then this\n> will always trigger and return 1, signaling a failure.\n\nAre, please disregard.  Yes, the above does the right thing already.\n"},{"id":"415082","messageId":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v2.git.1611320639.gitgitgadget@gmail.com","subject":"[PATCH v3 0/9] More index cleanups","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:10Z","receivedAt":"2021-01-23T19:59:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is based on ds/cache-tree-basics.\n\nHere are a few more cleanups that are vaguely related to the index. I\ndiscovered these while preparing my sparse-index RFC that I intend to send\nearly next week.\n\nThe biggest patch is the final one, which creates a test script for\ncomparing sparse-checkouts to full checkouts. There are some commands that\ndo not behave similarly. This script will be the backbone of my testing\nstrategy for the sparse-index by adding a new mode to compare\nsparse-checkouts with the two index types (full and sparse).\n\n\nUPDATES IN V3\n=============\n\n * Callers to cache_tree_update() no longer initialize the cache_tree in\n   advance.\n\n * Added a patch to update verify_cache() prototype.\n\n * Added missing \"pos + 1\" in fsmonitor.c.\n\n * Added a BUG() statement when repo->istate->repo is already populated, but\n   not equal to repo.\n\n * Cleaned up test_region pattern quoting. Thanks, Junio!\n\nThanks, -Stolee\n\nDerrick Stolee (9):\n  cache-tree: clean up cache_tree_update()\n  cache-tree: simplify verify_cache() prototype\n  cache-tree: extract subtree_pos()\n  fsmonitor: de-duplicate BUG()s around dirty bits\n  repository: add repo reference to index_state\n  name-hash: use trace2 regions for init\n  sparse-checkout: load sparse-checkout patterns\n  test-lib: test_region looks for trace2 regions\n  t1092: test interesting sparse-checkout scenarios\n\n builtin/checkout.c                       |   3 -\n builtin/sparse-checkout.c                |   5 -\n cache-tree.c                             |  38 +--\n cache-tree.h                             |   2 +\n cache.h                                  |   1 +\n dir.c                                    |  17 ++\n dir.h                                    |   2 +\n fsmonitor.c                              |  27 +-\n name-hash.c                              |   3 +\n repository.c                             |   6 +\n sequencer.c                              |   3 -\n t/t0500-progress-display.sh              |   3 +-\n t/t1092-sparse-checkout-compatibility.sh | 301 +++++++++++++++++++++++\n t/test-lib-functions.sh                  |  42 ++++\n unpack-trees.c                           |   8 +-\n 15 files changed, 408 insertions(+), 53 deletions(-)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\n\nbase-commit: a4b6d202caad83c6dc29abe9b17e53a1b3fb54a0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-839%2Fderrickstolee%2Fmore-index-cleanups-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-839/derrickstolee/more-index-cleanups-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/839\n\nRange-diff vs v2:\n\n  1:  f9dccaed0ac !  1:  bdc8ecca3d2 cache-tree: clean up cache_tree_update()\n     @@ Commit message\n          BUG() statement or returning with an error because future callers will\n          want to populate an empty cache-tree using this method.\n      \n     -    Also drop local variables that are used exactly once and can be found\n     -    directly from the 'istate' parameter.\n     +    Callers can also remove their conditional allocations of cache_tree.\n     +\n     +    Also drop local variables that can be found directly from the 'istate'\n     +    parameter.\n      \n          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n      \n     + ## builtin/checkout.c ##\n     +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n     + \t\t}\n     + \t}\n     + \n     +-\tif (!active_cache_tree)\n     +-\t\tactive_cache_tree = cache_tree();\n     +-\n     + \tif (!cache_tree_fully_valid(active_cache_tree))\n     + \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n     + \n     +\n       ## cache-tree.c ##\n      @@ cache-tree.c: static int update_one(struct cache_tree *it,\n       \n     @@ cache-tree.c: static int update_one(struct cache_tree *it,\n       \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n       \ttrace_performance_leave(\"cache_tree_update\");\n       \tif (i < 0)\n     +@@ cache-tree.c: static int write_index_as_tree_internal(struct object_id *oid,\n     + \t\tcache_tree_valid = 0;\n     + \t}\n     + \n     +-\tif (!index_state->cache_tree)\n     +-\t\tindex_state->cache_tree = cache_tree();\n     +-\n     + \tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n     + \t\treturn WRITE_TREE_UNMERGED_INDEX;\n     + \n     +\n     + ## sequencer.c ##\n     +@@ sequencer.c: static int do_recursive_merge(struct repository *r,\n     + \n     + static struct object_id *get_cache_tree_oid(struct index_state *istate)\n     + {\n     +-\tif (!istate->cache_tree)\n     +-\t\tistate->cache_tree = cache_tree();\n     +-\n     + \tif (!cache_tree_fully_valid(istate->cache_tree))\n     + \t\tif (cache_tree_update(istate, 0)) {\n     + \t\t\terror(_(\"unable to update cache tree\"));\n     +\n     + ## unpack-trees.c ##\n     +@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n     + \t\tif (!ret) {\n     + \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n     + \t\t\t\tcache_tree_verify(the_repository, &o->result);\n     +-\t\t\tif (!o->result.cache_tree)\n     +-\t\t\t\to->result.cache_tree = cache_tree();\n     + \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n     + \t\t\t\tcache_tree_update(&o->result,\n     + \t\t\t\t\t\t  WRITE_TREE_SILENT |\n  -:  ----------- >  2:  1b8b5680094 cache-tree: simplify verify_cache() prototype\n  2:  84323e04d08 =  3:  314b6b34f75 cache-tree: extract subtree_pos()\n  3:  31095f9aa0e !  4:  4e688d25f8c fsmonitor: de-duplicate BUG()s around dirty bits\n     @@ Commit message\n          cannot simplify it too much. However, the error string is identical in\n          each, so this simplifies things.\n      \n     +    Be sure to add one when checking if a position if valid, since the\n     +    minimum is a bound on the expected size.\n     +\n          The end result is that the code is simpler to read while also preserving\n          these assertions for developers in the FSMonitor space.\n      \n     @@ fsmonitor.c\n      -\tif (pos >= istate->cache_nr)\n      -\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n      -\t\t    (uintmax_t)pos, istate->cache_nr);\n     -+\tassert_index_minimum(istate, pos);\n     ++\tassert_index_minimum(istate, pos + 1);\n       \n       \tce = istate->cache[pos];\n       \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n  4:  a0d89d7a973 !  5:  6373997e05c repository: add repo reference to index_state\n     @@ Commit message\n          repository, add a 'repo' pointer to struct index_state that allows\n          access to this repository.\n      \n     +    Add a BUG() statement if the repo already has an index, and the index\n     +    already has a repo, but somehow the index points to a different repo.\n     +\n          This will prevent future changes from needing to pass an additional\n          'struct repository *repo' parameter and instead rely only on the 'struct\n          index_state *istate' parameter.\n     @@ repository.c: int repo_read_index(struct repository *repo)\n      +\t/* Complete the double-reference */\n      +\tif (!repo->index->repo)\n      +\t\trepo->index->repo = repo;\n     ++\telse if (repo->index->repo != repo)\n     ++\t\tBUG(\"repo's index should point back at itself\");\n      +\n       \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n       }\n  5:  bc092f5c703 =  6:  9b545d7dbec name-hash: use trace2 regions for init\n  6:  04d1daf7222 =  7:  554cc7647e6 sparse-checkout: load sparse-checkout patterns\n  7:  8832ce84623 !  8:  b37181bdec4 test-lib: test_region looks for trace2 regions\n     @@ t/test-lib-functions.sh: test_subcommand () {\n      +\t\tshift\n      +\tfi\n      +\n     -+\tgrep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n     ++\tgrep -e\t'\"region_enter\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n      +\texitcode=$?\n      +\n     -+\tif test $exitcode != $expect_exit\n     ++\tif test $exitcode != $expect_exit = 1]\n      +\tthen\n      +\t\treturn 1\n      +\tfi\n      +\n     -+\tgrep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n     ++\tgrep -e\t'\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n      +\texitcode=$?\n      +\n     -+\tif test $exitcode != $expect_exit\n     ++\tif test $exitcode != $expect_exit = 1]\n      +\tthen\n      +\t\treturn 1\n      +\tfi\n     ++\n     ++\treturn 0\n      +}\n  8:  984458007ed !  9:  72f925353d3 t1092: test interesting sparse-checkout scenarios\n     @@ t/t1092-sparse-checkout-compatibility.sh (new)\n      +\t\techo a >a &&\n      +\t\techo \"after deep\" >e &&\n      +\t\techo \"after folder1\" >g &&\n     ++\t\techo \"after x\" >z &&\n      +\t\tmkdir folder1 folder2 deep x &&\n      +\t\tmkdir deep/deeper1 deep/deeper2 &&\n      +\t\tmkdir deep/deeper1/deepest &&\n     @@ t/t1092-sparse-checkout-compatibility.sh (new)\n      +\t\techo \"after deepest\" >deep/deeper1/e &&\n      +\t\tcp a folder1 &&\n      +\t\tcp a folder2 &&\n     ++\t\tcp a x &&\n      +\t\tcp a deep &&\n      +\t\tcp a deep/deeper1 &&\n      +\t\tcp a deep/deeper2 &&\n      +\t\tcp a deep/deeper1/deepest &&\n     ++\t\tcp -r deep/deeper1/deepest deep/deeper2 &&\n      +\t\tgit add . &&\n      +\t\tgit commit -m \"initial commit\" &&\n      +\t\tgit checkout -b base &&\n\n-- \ngitgitgadget\n"},{"id":"415083","messageId":"314b6b34f759c3a0e0d361bc08b254960a4d28e4.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 3/9] cache-tree: extract subtree_pos()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:13Z","receivedAt":"2021-01-23T19:59:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThis method will be helpful to use outside of cache-tree.c in a later\nfeature. The implementation is subtle due to subtree_name_cmp() sorting\nby length and then lexicographically.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 6 +++---\n cache-tree.h | 2 ++\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex acac6d58c37..2fb483d3c08 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,\n \treturn memcmp(one, two, onelen);\n }\n \n-static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen)\n {\n \tstruct cache_tree_sub **down = it->down;\n \tint lo, hi;\n@@ -72,7 +72,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,\n \t\t\t\t\t   int create)\n {\n \tstruct cache_tree_sub *down;\n-\tint pos = subtree_pos(it, path, pathlen);\n+\tint pos = cache_tree_subtree_pos(it, path, pathlen);\n \tif (0 <= pos)\n \t\treturn it->down[pos];\n \tif (!create)\n@@ -123,7 +123,7 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)\n \tit->entry_count = -1;\n \tif (!*slash) {\n \t\tint pos;\n-\t\tpos = subtree_pos(it, path, namelen);\n+\t\tpos = cache_tree_subtree_pos(it, path, namelen);\n \t\tif (0 <= pos) {\n \t\t\tcache_tree_free(&it->down[pos]->cache_tree);\n \t\t\tfree(it->down[pos]);\ndiff --git a/cache-tree.h b/cache-tree.h\nindex 639bfa5340e..8efeccebfc9 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -27,6 +27,8 @@ void cache_tree_free(struct cache_tree **);\n void cache_tree_invalidate_path(struct index_state *, const char *);\n struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);\n \n+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);\n+\n void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n-- \ngitgitgadget\n\n"},{"id":"415084","messageId":"1b8b56800948339c0e0387555698bdfdc80a19ad.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:12Z","receivedAt":"2021-01-23T19:59:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe verify_cache() method takes an array of cache entries and a count,\nbut these are always provided directly from a struct index_state. Use\na pointer to the full structure instead.\n\nThere is a subtle point when istate->cache_nr is zero that subtracting\none will underflow. This triggers a failure in t0000-basic.sh, among\nothers. Use \"i + 1 < istate->cache_nr\" to avoid these strange\ncomparisons. Convert i to be unsigned as well, which also removes the\npotential signed overflow in the unlikely case that cache_nr is over 2.1\nbillion entries. The 'funny' variable has a maximum value of 11, so\nmaking it unsigned does not change anything of importance.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache-tree.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 60b6aefbf51..acac6d58c37 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -151,16 +151,15 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)\n \t\tistate->cache_changed |= CACHE_TREE_CHANGED;\n }\n \n-static int verify_cache(struct cache_entry **cache,\n-\t\t\tint entries, int flags)\n+static int verify_cache(struct index_state *istate, int flags)\n {\n-\tint i, funny;\n+\tunsigned i, funny;\n \tint silent = flags & WRITE_TREE_SILENT;\n \n \t/* Verify that the tree is merged */\n \tfunny = 0;\n-\tfor (i = 0; i < entries; i++) {\n-\t\tconst struct cache_entry *ce = cache[i];\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tconst struct cache_entry *ce = istate->cache[i];\n \t\tif (ce_stage(ce)) {\n \t\t\tif (silent)\n \t\t\t\treturn -1;\n@@ -180,13 +179,13 @@ static int verify_cache(struct cache_entry **cache,\n \t * stage 0 entries.\n \t */\n \tfunny = 0;\n-\tfor (i = 0; i < entries - 1; i++) {\n+\tfor (i = 0; i + 1 < istate->cache_nr; i++) {\n \t\t/* path/file always comes after path because of the way\n \t\t * the cache is sorted.  Also path can appear only once,\n \t\t * which means conflicting one would immediately follow.\n \t\t */\n-\t\tconst struct cache_entry *this_ce = cache[i];\n-\t\tconst struct cache_entry *next_ce = cache[i + 1];\n+\t\tconst struct cache_entry *this_ce = istate->cache[i];\n+\t\tconst struct cache_entry *next_ce = istate->cache[i + 1];\n \t\tconst char *this_name = this_ce->name;\n \t\tconst char *next_name = next_ce->name;\n \t\tint this_len = ce_namelen(this_ce);\n@@ -438,7 +437,7 @@ int cache_tree_update(struct index_state *istate, int flags)\n {\n \tint skip, i;\n \n-\ti = verify_cache(istate->cache, istate->cache_nr, flags);\n+\ti = verify_cache(istate, flags);\n \n \tif (i)\n \t\treturn i;\n-- \ngitgitgadget\n\n"},{"id":"415085","messageId":"4e688d25f8c7949dd3edbdb47b924450d9a50796.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 4/9] fsmonitor: de-duplicate BUG()s around dirty bits","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:14Z","receivedAt":"2021-01-23T19:59:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe index has an fsmonitor_dirty bitmap that records which index entries\nare \"dirty\" based on the response from the FSMonitor. If this bitmap\never grows larger than the index, then there was an error in how it was\nconstructed, and it was probably a developer's bug.\n\nThere are several BUG() statements that are very similar, so replace\nthese uses with a simpler assert_index_minimum(). Since there is one\ncaller that uses a custom 'pos' value instead of the bit_size member, we\ncannot simplify it too much. However, the error string is identical in\neach, so this simplifies things.\n\nBe sure to add one when checking if a position if valid, since the\nminimum is a bound on the expected size.\n\nThe end result is that the code is simpler to read while also preserving\nthese assertions for developers in the FSMonitor space.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n fsmonitor.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex ca031c3abb8..fe9e9d7baf4 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -13,14 +13,19 @@\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n+static void assert_index_minimum(struct index_state *istate, size_t pos)\n+{\n+\tif (pos > istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n+\t\t    (uintmax_t)pos, istate->cache_nr);\n+}\n+\n static void fsmonitor_ewah_callback(size_t pos, void *is)\n {\n \tstruct index_state *istate = (struct index_state *)is;\n \tstruct cache_entry *ce;\n \n-\tif (pos >= istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n-\t\t    (uintmax_t)pos, istate->cache_nr);\n+\tassert_index_minimum(istate, pos + 1);\n \n \tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n@@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n@@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (!istate->split_index &&\n-\t    istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\tif (!istate->split_index)\n+\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n@@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t}\n \n \t\t\t/* Mark all previously saved entries as dirty */\n-\t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n-\t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n-\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n+\t\t\tassert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n \t\t\trefresh_fsmonitor(istate);\n-- \ngitgitgadget\n\n"},{"id":"415086","messageId":"bdc8ecca3d2a2fbdb1d8bf10222382c14986aa56.1611431899.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 1/9] cache-tree: clean up cache_tree_update()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:11Z","receivedAt":"2021-01-23T19:59:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMake the method safer by allocating a cache_tree member for the given\nindex_state if it is not already present. This is preferrable to a\nBUG() statement or returning with an error because future callers will\nwant to populate an empty cache-tree using this method.\n\nCallers can also remove their conditional allocations of cache_tree.\n\nAlso drop local variables that can be found directly from the 'istate'\nparameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/checkout.c |  3 ---\n cache-tree.c       | 17 +++++++++--------\n sequencer.c        |  3 ---\n unpack-trees.c     |  2 --\n 4 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c9ba23c2794..2d6550bc3c8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -821,9 +821,6 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t}\n \t}\n \n-\tif (!active_cache_tree)\n-\t\tactive_cache_tree = cache_tree();\n-\n \tif (!cache_tree_fully_valid(active_cache_tree))\n \t\tcache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n \ndiff --git a/cache-tree.c b/cache-tree.c\nindex 3f1a8d4f1b7..60b6aefbf51 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,\n \n int cache_tree_update(struct index_state *istate, int flags)\n {\n-\tstruct cache_tree *it = istate->cache_tree;\n-\tstruct cache_entry **cache = istate->cache;\n-\tint entries = istate->cache_nr;\n-\tint skip, i = verify_cache(cache, entries, flags);\n+\tint skip, i;\n+\n+\ti = verify_cache(istate->cache, istate->cache_nr, flags);\n \n \tif (i)\n \t\treturn i;\n+\n+\tif (!istate->cache_tree)\n+\t\tistate->cache_tree = cache_tree();\n+\n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n-\ti = update_one(it, cache, entries, \"\", 0, &skip, flags);\n+\ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n+\t\t       \"\", 0, &skip, flags);\n \ttrace2_region_leave(\"cache_tree\", \"update\", the_repository);\n \ttrace_performance_leave(\"cache_tree_update\");\n \tif (i < 0)\n@@ -635,9 +639,6 @@ static int write_index_as_tree_internal(struct object_id *oid,\n \t\tcache_tree_valid = 0;\n \t}\n \n-\tif (!index_state->cache_tree)\n-\t\tindex_state->cache_tree = cache_tree();\n-\n \tif (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n \t\treturn WRITE_TREE_UNMERGED_INDEX;\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 8909a467700..aa3e4c81cf0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -679,9 +679,6 @@ static int do_recursive_merge(struct repository *r,\n \n static struct object_id *get_cache_tree_oid(struct index_state *istate)\n {\n-\tif (!istate->cache_tree)\n-\t\tistate->cache_tree = cache_tree();\n-\n \tif (!cache_tree_fully_valid(istate->cache_tree))\n \t\tif (cache_tree_update(istate, 0)) {\n \t\t\terror(_(\"unable to update cache tree\"));\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex af6e9b9c2fd..a810b79657e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1726,8 +1726,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\tif (!ret) {\n \t\t\tif (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n \t\t\t\tcache_tree_verify(the_repository, &o->result);\n-\t\t\tif (!o->result.cache_tree)\n-\t\t\t\to->result.cache_tree = cache_tree();\n \t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n \t\t\t\tcache_tree_update(&o->result,\n \t\t\t\t\t\t  WRITE_TREE_SILENT |\n-- \ngitgitgadget\n\n"},{"id":"415087","messageId":"6373997e05c04f57e441355eac94ca03c634e3fb.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 5/9] repository: add repo reference to index_state","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:15Z","receivedAt":"2021-01-23T20:00:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nIt will be helpful to add behavior to index operations that might\ntrigger an object lookup. Since each index belongs to a specific\nrepository, add a 'repo' pointer to struct index_state that allows\naccess to this repository.\n\nAdd a BUG() statement if the repo already has an index, and the index\nalready has a repo, but somehow the index points to a different repo.\n\nThis will prevent future changes from needing to pass an additional\n'struct repository *repo' parameter and instead rely only on the 'struct\nindex_state *istate' parameter.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n cache.h      | 1 +\n repository.c | 6 ++++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 71097657489..f9c7a603841 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -328,6 +328,7 @@ struct index_state {\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\n+\tstruct repository *repo;\n };\n \n /* Name hashing */\ndiff --git a/repository.c b/repository.c\nindex a4174ddb062..c98298acd01 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -264,6 +264,12 @@ int repo_read_index(struct repository *repo)\n \tif (!repo->index)\n \t\trepo->index = xcalloc(1, sizeof(*repo->index));\n \n+\t/* Complete the double-reference */\n+\tif (!repo->index->repo)\n+\t\trepo->index->repo = repo;\n+\telse if (repo->index->repo != repo)\n+\t\tBUG(\"repo's index should point back at itself\");\n+\n \treturn read_index_from(repo->index, repo->index_file, repo->gitdir);\n }\n \n-- \ngitgitgadget\n\n"},{"id":"415088","messageId":"9b545d7dbecc6f071e58787e9d4339f59ac4e972.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 6/9] name-hash: use trace2 regions for init","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:16Z","receivedAt":"2021-01-23T20:00:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe lazy_init_name_hash() populates a hashset with all filenames and\nanother with all directories represented in the index. This is run only\nif we need to use the hashsets to check for existence or case-folding\nrenames.\n\nPlace trace2 regions where there is already a performance trace.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n name-hash.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex 5d3c7b12c18..4e03fac9bb1 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -7,6 +7,7 @@\n  */\n #include \"cache.h\"\n #include \"thread-utils.h\"\n+#include \"trace2.h\"\n \n struct dir_entry {\n \tstruct hashmap_entry ent;\n@@ -577,6 +578,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \tif (istate->name_hash_initialized)\n \t\treturn;\n \ttrace_performance_enter();\n+\ttrace2_region_enter(\"index\", \"name-hash-init\", istate->repo);\n \thashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);\n \thashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);\n \n@@ -597,6 +599,7 @@ static void lazy_init_name_hash(struct index_state *istate)\n \t}\n \n \tistate->name_hash_initialized = 1;\n+\ttrace2_region_leave(\"index\", \"name-hash-init\", istate->repo);\n \ttrace_performance_leave(\"initialize name hash\");\n }\n \n-- \ngitgitgadget\n\n"},{"id":"415089","messageId":"b37181bdec43cfc798740f2bdd19f6d2482beb26.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 8/9] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:18Z","receivedAt":"2021-01-23T20:00:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMost test cases can verify Git's behavior using input/output\nexpectations or changes to the .git directory. However, sometimes we\nwant to check that Git did or did not run a certain section of code.\nThis is particularly important for performance-only features that we\nwant to ensure have been enabled in certain cases.\n\nAdd a new 'test_region' function that checks if a trace2 region was\nentered and left in a given trace2 event log.\n\nThere is one existing test (t0500-progress-display.sh) that performs\nthis check already, so use the helper function instead. Note that this\nchanges the expectations slightly. The old test (incorrectly) used two\npatterns for the 'grep' invocation, but this performs an OR of the\npatterns, not an AND. This means that as long as one region_enter event\nwas logged, the test would succeed, even if it was not due to the\nprogress category.\n\nMore uses will be added in a later change.\n\nt6423-merge-rename-directories.sh also greps for region_enter lines, but\nit verifies the number of such lines, which is not the same as an\nexistence check.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t0500-progress-display.sh |  3 +--\n t/test-lib-functions.sh     | 42 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 1ed1df351cb..84cce345e7d 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '\n \t\t\"Working hard\" <in 2>stderr &&\n \n \t# t0212/parse_events.perl intentionally omits regions and data.\n-\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n-\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\ttest_region progress \"Working hard\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a9..3f425deba18 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1655,3 +1655,45 @@ test_subcommand () {\n \t\tgrep \"\\[$expr\\]\"\n \tfi\n }\n+\n+# Check that the given command was invoked as part of the\n+# trace2-format trace on stdin.\n+#\n+#\ttest_region [!] <category> <label> git <command> <args>...\n+#\n+# For example, to look for trace2_region_enter(\"index\", \"do_read_index\", repo)\n+# in an invocation of \"git checkout HEAD~1\", run\n+#\n+#\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+#\t\tgit checkout HEAD~1 &&\n+#\ttest_region index do_read_index <trace.txt\n+#\n+# If the first parameter passed is !, this instead checks that\n+# the given region was not entered.\n+#\n+test_region () {\n+\tlocal expect_exit=0\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\texpect_exit=1\n+\t\tshift\n+\tfi\n+\n+\tgrep -e\t'\"region_enter\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit = 1]\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tgrep -e\t'\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit = 1]\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\treturn 0\n+}\n-- \ngitgitgadget\n\n"},{"id":"415090","messageId":"554cc7647e63008e1e2cf2ae3813412e16cfcce7.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 7/9] sparse-checkout: load sparse-checkout patterns","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:17Z","receivedAt":"2021-01-23T20:01:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nA future feature will want to load the sparse-checkout patterns into a\npattern_list, but the current mechanism to do so is a bit complicated.\nThis is made difficult due to needing to find the sparse-checkout file\nin different ways throughout the codebase.\n\nThe logic implemented in the new get_sparse_checkout_patterns() was\nduplicated in populate_from_existing_patterns() in unpack-trees.c. Use\nthe new method instead, keeping the logic around handling the struct\nunpack_trees_options.\n\nThe callers to get_sparse_checkout_filename() in\nbuiltin/sparse-checkout.c manipulate the sparse-checkout file directly,\nso it is not appropriate to replace logic in that file with\nget_sparse_checkout_patterns().\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/sparse-checkout.c |  5 -----\n dir.c                     | 17 +++++++++++++++++\n dir.h                     |  2 ++\n unpack-trees.c            |  6 +-----\n 4 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex e3140db2a0a..2306a9ad98e 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {\n \tNULL\n };\n \n-static char *get_sparse_checkout_filename(void)\n-{\n-\treturn git_pathdup(\"info/sparse-checkout\");\n-}\n-\n static void write_patterns_to_file(FILE *fp, struct pattern_list *pl)\n {\n \tint i;\ndiff --git a/dir.c b/dir.c\nindex d637461da5c..d153a63bbd1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2998,6 +2998,23 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t}\n }\n \n+char *get_sparse_checkout_filename(void)\n+{\n+\treturn git_pathdup(\"info/sparse-checkout\");\n+}\n+\n+int get_sparse_checkout_patterns(struct pattern_list *pl)\n+{\n+\tint res;\n+\tchar *sparse_filename = get_sparse_checkout_filename();\n+\n+\tpl->use_cone_patterns = core_sparse_checkout_cone;\n+\tres = add_patterns_from_file_to_list(sparse_filename, \"\", 0, pl, NULL);\n+\n+\tfree(sparse_filename);\n+\treturn res;\n+}\n+\n int remove_path(const char *name)\n {\n \tchar *slash;\ndiff --git a/dir.h b/dir.h\nindex a3c40dec516..facfae47402 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -448,6 +448,8 @@ int is_empty_dir(const char *dir);\n \n void setup_standard_excludes(struct dir_struct *dir);\n \n+char *get_sparse_checkout_filename(void);\n+int get_sparse_checkout_patterns(struct pattern_list *pl);\n \n /* Constants for remove_dir_recursively: */\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a810b79657e..f5f668f532d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1549,14 +1549,10 @@ static void mark_new_skip_worktree(struct pattern_list *pl,\n static void populate_from_existing_patterns(struct unpack_trees_options *o,\n \t\t\t\t\t    struct pattern_list *pl)\n {\n-\tchar *sparse = git_pathdup(\"info/sparse-checkout\");\n-\n-\tpl->use_cone_patterns = core_sparse_checkout_cone;\n-\tif (add_patterns_from_file_to_list(sparse, \"\", 0, pl, NULL) < 0)\n+\tif (get_sparse_checkout_patterns(pl) < 0)\n \t\to->skip_sparse_checkout = 1;\n \telse\n \t\to->pl = pl;\n-\tfree(sparse);\n }\n \n \n-- \ngitgitgadget\n\n"},{"id":"415091","messageId":"72f925353d3f3992ab6a98df2b0752c019d11338.1611431900.git.gitgitgadget@gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"[PATCH v3 9/9] t1092: test interesting sparse-checkout scenarios","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-23T19:58:19Z","receivedAt":"2021-01-23T20:01:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThese also document some behaviors that differ from a full checkout, and\npossibly in a way that is not intended.\n\nThe test is designed to be run with \"--run=1,X\" where 'X' is an\ninteresting test case. Each test uses 'init_repos' to reset the full and\nsparse copies of the initial-repo that is created by the first test\ncase. This also makes it possible to have test cases leave the working\ndirectory or index in unusual states without disturbing later cases.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t1092-sparse-checkout-compatibility.sh | 301 +++++++++++++++++++++++\n 1 file changed, 301 insertions(+)\n create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nnew file mode 100755\nindex 00000000000..8cd3e5a8d22\n--- /dev/null\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -0,0 +1,301 @@\n+#!/bin/sh\n+\n+test_description='compare full workdir to sparse workdir'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init initial-repo &&\n+\t(\n+\t\tcd initial-repo &&\n+\t\techo a >a &&\n+\t\techo \"after deep\" >e &&\n+\t\techo \"after folder1\" >g &&\n+\t\techo \"after x\" >z &&\n+\t\tmkdir folder1 folder2 deep x &&\n+\t\tmkdir deep/deeper1 deep/deeper2 &&\n+\t\tmkdir deep/deeper1/deepest &&\n+\t\techo \"after deeper1\" >deep/e &&\n+\t\techo \"after deepest\" >deep/deeper1/e &&\n+\t\tcp a folder1 &&\n+\t\tcp a folder2 &&\n+\t\tcp a x &&\n+\t\tcp a deep &&\n+\t\tcp a deep/deeper1 &&\n+\t\tcp a deep/deeper2 &&\n+\t\tcp a deep/deeper1/deepest &&\n+\t\tcp -r deep/deeper1/deepest deep/deeper2 &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit checkout -b base &&\n+\t\tfor dir in folder1 folder2 deep\n+\t\tdo\n+\t\t\tgit checkout -b update-$dir &&\n+\t\t\techo \"updated $dir\" >$dir/a &&\n+\t\t\tgit commit -a -m \"update $dir\" || return 1\n+\t\tdone &&\n+\n+\t\tgit checkout -b rename-base base &&\n+\t\techo >folder1/larger-content <<-\\EOF &&\n+\t\tmatching\n+\t\tlines\n+\t\thelp\n+\t\tinexact\n+\t\trenames\n+\t\tEOF\n+\t\tcp folder1/larger-content folder2/ &&\n+\t\tcp folder1/larger-content deep/deeper1/ &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"add interesting rename content\" &&\n+\n+\t\tgit checkout -b rename-out-to-out rename-base &&\n+\t\tmv folder1/a folder2/b &&\n+\t\tmv folder1/larger-content folder2/edited-content &&\n+\t\techo >>folder2/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to folder2/...\" &&\n+\n+\t\tgit checkout -b rename-out-to-in rename-base &&\n+\t\tmv folder1/a deep/deeper1/b &&\n+\t\tmv folder1/larger-content deep/deeper1/edited-content &&\n+\t\techo >>deep/deeper1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename folder1/... to deep/deeper1/...\" &&\n+\n+\t\tgit checkout -b rename-in-to-out rename-base &&\n+\t\tmv deep/deeper1/a folder1/b &&\n+\t\tmv deep/deeper1/larger-content folder1/edited-content &&\n+\t\techo >>folder1/edited-content &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"rename deep/deeper1/... to folder1/...\" &&\n+\n+\t\tgit checkout -b deepest base &&\n+\t\techo \"updated deepest\" >deep/deeper1/deepest/a &&\n+\t\tgit commit -a -m \"update deepest\" &&\n+\n+\t\tgit checkout -f base &&\n+\t\tgit reset --hard\n+\t)\n+'\n+\n+init_repos () {\n+\trm -rf full-checkout sparse-checkout sparse-index &&\n+\n+\t# create repos in initial state\n+\tcp -r initial-repo full-checkout &&\n+\tgit -C full-checkout reset --hard &&\n+\n+\tcp -r initial-repo sparse-checkout &&\n+\tgit -C sparse-checkout reset --hard &&\n+\tgit -C sparse-checkout sparse-checkout init --cone &&\n+\n+\t# initialize sparse-checkout definitions\n+\tgit -C sparse-checkout sparse-checkout set deep\n+}\n+\n+run_on_sparse () {\n+\t(\n+\t\tcd sparse-checkout &&\n+\t\t$* >../sparse-checkout-out 2>../sparse-checkout-err\n+\t)\n+}\n+\n+run_on_all () {\n+\t(\n+\t\tcd full-checkout &&\n+\t\t$* >../full-checkout-out 2>../full-checkout-err\n+\t) &&\n+\trun_on_sparse $*\n+}\n+\n+test_all_match () {\n+\trun_on_all $* &&\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err\n+}\n+\n+test_expect_success 'status with options' '\n+\tinit_repos &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\trun_on_all \"touch README.md\" &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git status --porcelain=v2 -z -u &&\n+\ttest_all_match git status --porcelain=v2 -uno\n+'\n+\n+test_expect_success 'add, commit, checkout' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Add README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents README.md\" &&\n+\n+\ttest_all_match git add -A &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"Extend README.md\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout - &&\n+\n+\trun_on_all \"../edit-contents deep/newfile\" &&\n+\n+\ttest_all_match git status --porcelain=v2 -uno &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git add . &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git commit -m \"add deep/newfile\" &&\n+\n+\ttest_all_match git checkout HEAD~1 &&\n+\ttest_all_match git checkout -\n+'\n+\n+test_expect_success 'checkout and reset --hard' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout update-deep &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git checkout -b reset-test &&\n+\ttest_all_match git reset --hard deepest &&\n+\ttest_all_match git reset --hard update-folder1 &&\n+\ttest_all_match git reset --hard update-folder2\n+'\n+\n+test_expect_success 'diff --staged' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>README.md\n+\tEOF\n+\trun_on_all \"../edit-contents\" &&\n+\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged &&\n+\ttest_all_match git add README.md &&\n+\ttest_all_match git diff &&\n+\ttest_all_match git diff --staged\n+'\n+\n+test_expect_success 'diff with renames' '\n+\tinit_repos &&\n+\n+\tfor branch in rename-out-to-out rename-out-to-in rename-in-to-out\n+\tdo\n+\t\ttest_all_match git checkout rename-base &&\n+\t\ttest_all_match git checkout $branch -- .&&\n+\t\ttest_all_match git diff --staged --no-renames &&\n+\t\ttest_all_match git diff --staged --find-renames || return 1\n+\tdone\n+'\n+\n+test_expect_success 'log with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git log -- a &&\n+\ttest_all_match git log -- folder1/a &&\n+\ttest_all_match git log -- folder2/a &&\n+\ttest_all_match git log -- deep/a &&\n+\ttest_all_match git log -- deep/deeper1/a &&\n+\ttest_all_match git log -- deep/deeper1/deepest/a &&\n+\n+\ttest_all_match git checkout update-folder1 &&\n+\ttest_all_match git log -- folder1/a\n+'\n+\n+test_expect_success 'blame with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame a &&\n+\ttest_all_match git blame deep/a &&\n+\ttest_all_match git blame deep/deeper1/a &&\n+\ttest_all_match git blame deep/deeper1/deepest/a\n+'\n+\n+# TODO: blame currently does not support blaming files outside of the\n+# sparse definition. It complains that the file doesn't exist locally.\n+test_expect_failure 'blame with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_all_match git blame folder1/a &&\n+\ttest_all_match git blame folder2/a &&\n+\ttest_all_match git blame deep/deeper2/a &&\n+\ttest_all_match git blame deep/deeper2/deepest/a\n+'\n+\n+# TODO: reset currently does not behave as expected when in a\n+# sparse-checkout.\n+test_expect_failure 'checkout and reset (mixed)' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b reset-test update-deep &&\n+\ttest_all_match git reset deepest &&\n+\ttest_all_match git reset update-folder1 &&\n+\ttest_all_match git reset update-folder2\n+'\n+\n+test_expect_success 'merge' '\n+\tinit_repos &&\n+\n+\ttest_all_match git checkout -b merge update-deep &&\n+\ttest_all_match git merge -m \"folder1\" update-folder1 &&\n+\ttest_all_match git rev-parse HEAD^{tree} &&\n+\ttest_all_match git merge -m \"folder2\" update-folder2 &&\n+\ttest_all_match git rev-parse HEAD^{tree}\n+'\n+\n+test_expect_success 'merge with outside renames' '\n+\tinit_repos &&\n+\n+\tfor type in out-to-out out-to-in in-to-out\n+\tdo\n+\t\ttest_all_match git reset --hard &&\n+\t\ttest_all_match git checkout -f -b merge-$type update-deep &&\n+\t\ttest_all_match git merge -m \"$type\" rename-$type &&\n+\t\ttest_all_match git rev-parse HEAD^{tree} || return 1\n+\tdone\n+'\n+\n+test_expect_success 'clean' '\n+\tinit_repos &&\n+\n+\techo bogus >>.gitignore &&\n+\trun_on_all cp ../.gitignore . &&\n+\ttest_all_match git add .gitignore &&\n+\ttest_all_match git commit -m ignore-bogus-files &&\n+\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all touch folder1/bogus &&\n+\n+\ttest_all_match git status --porcelain=v2 &&\n+\ttest_all_match git clean -f &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_all_match git clean -xdf &&\n+\ttest_all_match git status --porcelain=v2 &&\n+\n+\ttest_path_is_dir sparse-checkout/folder1\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"415092","messageId":"CABPp-BEsoWs5ZEhS0KTHankzc8eUdmpn0uoF7t1ZtN8b2gwvBA@mail.gmail.com","threadId":"55018","inReplyTo":"1b8b56800948339c0e0387555698bdfdc80a19ad.1611431900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-23T20:24:30Z","receivedAt":"2021-01-23T20:25:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> The verify_cache() method takes an array of cache entries and a count,\n> but these are always provided directly from a struct index_state. Use\n> a pointer to the full structure instead.\n>\n> There is a subtle point when istate->cache_nr is zero that subtracting\n> one will underflow. This triggers a failure in t0000-basic.sh, among\n> others. Use \"i + 1 < istate->cache_nr\" to avoid these strange\n> comparisons. Convert i to be unsigned as well, which also removes the\n> potential signed overflow in the unlikely case that cache_nr is over 2.1\n> billion entries. The 'funny' variable has a maximum value of 11, so\n\nAND a minimum value of 0 (which is important for the type change to be valid).\n\n> making it unsigned does not change anything of importance.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  cache-tree.c | 17 ++++++++---------\n>  1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 60b6aefbf51..acac6d58c37 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -151,16 +151,15 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)\n>                 istate->cache_changed |= CACHE_TREE_CHANGED;\n>  }\n>\n> -static int verify_cache(struct cache_entry **cache,\n> -                       int entries, int flags)\n> +static int verify_cache(struct index_state *istate, int flags)\n>  {\n> -       int i, funny;\n> +       unsigned i, funny;\n>         int silent = flags & WRITE_TREE_SILENT;\n>\n>         /* Verify that the tree is merged */\n>         funny = 0;\n> -       for (i = 0; i < entries; i++) {\n> -               const struct cache_entry *ce = cache[i];\n> +       for (i = 0; i < istate->cache_nr; i++) {\n> +               const struct cache_entry *ce = istate->cache[i];\n>                 if (ce_stage(ce)) {\n>                         if (silent)\n>                                 return -1;\n> @@ -180,13 +179,13 @@ static int verify_cache(struct cache_entry **cache,\n>          * stage 0 entries.\n>          */\n>         funny = 0;\n> -       for (i = 0; i < entries - 1; i++) {\n> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n>                 /* path/file always comes after path because of the way\n>                  * the cache is sorted.  Also path can appear only once,\n>                  * which means conflicting one would immediately follow.\n>                  */\n> -               const struct cache_entry *this_ce = cache[i];\n> -               const struct cache_entry *next_ce = cache[i + 1];\n> +               const struct cache_entry *this_ce = istate->cache[i];\n> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n>                 const char *this_name = this_ce->name;\n>                 const char *next_name = next_ce->name;\n>                 int this_len = ce_namelen(this_ce);\n> @@ -438,7 +437,7 @@ int cache_tree_update(struct index_state *istate, int flags)\n>  {\n>         int skip, i;\n>\n> -       i = verify_cache(istate->cache, istate->cache_nr, flags);\n> +       i = verify_cache(istate, flags);\n>\n>         if (i)\n>                 return i;\n> --\n> gitgitgadget\n\nMakes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\nin the commit message; made it easier to read through quickly.  I'm\ncurious if it deserves a comment in the code too, since it does feel\nslightly unusual.\n"},{"id":"415093","messageId":"CABPp-BHFiXLOS4Vbm156+JxZw=t2P_Fzf8E3p4u0iwCv28_Rdw@mail.gmail.com","threadId":"55018","inReplyTo":"pull.839.v3.git.1611431899.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/9] More index cleanups","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-23T20:29:21Z","receivedAt":"2021-01-23T20:30:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> This is based on ds/cache-tree-basics.\n>\n> Here are a few more cleanups that are vaguely related to the index. I\n> discovered these while preparing my sparse-index RFC that I intend to send\n> early next week.\n>\n> The biggest patch is the final one, which creates a test script for\n> comparing sparse-checkouts to full checkouts. There are some commands that\n> do not behave similarly. This script will be the backbone of my testing\n> strategy for the sparse-index by adding a new mode to compare\n> sparse-checkouts with the two index types (full and sparse).\n>\n>\n> UPDATES IN V3\n> =============\n>\n>  * Callers to cache_tree_update() no longer initialize the cache_tree in\n>    advance.\n>\n>  * Added a patch to update verify_cache() prototype.\n>\n>  * Added missing \"pos + 1\" in fsmonitor.c.\n>\n>  * Added a BUG() statement when repo->istate->repo is already populated, but\n>    not equal to repo.\n>\n>  * Cleaned up test_region pattern quoting. Thanks, Junio!\n>\n> Thanks, -Stolee\n>\n> Derrick Stolee (9):\n>   cache-tree: clean up cache_tree_update()\n>   cache-tree: simplify verify_cache() prototype\n>   cache-tree: extract subtree_pos()\n>   fsmonitor: de-duplicate BUG()s around dirty bits\n>   repository: add repo reference to index_state\n>   name-hash: use trace2 regions for init\n>   sparse-checkout: load sparse-checkout patterns\n>   test-lib: test_region looks for trace2 regions\n>   t1092: test interesting sparse-checkout scenarios\n>\n>  builtin/checkout.c                       |   3 -\n>  builtin/sparse-checkout.c                |   5 -\n>  cache-tree.c                             |  38 +--\n>  cache-tree.h                             |   2 +\n>  cache.h                                  |   1 +\n>  dir.c                                    |  17 ++\n>  dir.h                                    |   2 +\n>  fsmonitor.c                              |  27 +-\n>  name-hash.c                              |   3 +\n>  repository.c                             |   6 +\n>  sequencer.c                              |   3 -\n>  t/t0500-progress-display.sh              |   3 +-\n>  t/t1092-sparse-checkout-compatibility.sh | 301 +++++++++++++++++++++++\n>  t/test-lib-functions.sh                  |  42 ++++\n>  unpack-trees.c                           |   8 +-\n>  15 files changed, 408 insertions(+), 53 deletions(-)\n>  create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n>\n>\n> base-commit: a4b6d202caad83c6dc29abe9b17e53a1b3fb54a0\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-839%2Fderrickstolee%2Fmore-index-cleanups-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-839/derrickstolee/more-index-cleanups-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/839\n>\n> Range-diff vs v2:\n>\n>   1:  f9dccaed0ac !  1:  bdc8ecca3d2 cache-tree: clean up cache_tree_update()\n>      @@ Commit message\n>           BUG() statement or returning with an error because future callers will\n>           want to populate an empty cache-tree using this method.\n>\n>      -    Also drop local variables that are used exactly once and can be found\n>      -    directly from the 'istate' parameter.\n>      +    Callers can also remove their conditional allocations of cache_tree.\n>      +\n>      +    Also drop local variables that can be found directly from the 'istate'\n>      +    parameter.\n>\n>           Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>\n>      + ## builtin/checkout.c ##\n>      +@@ builtin/checkout.c: static int merge_working_tree(const struct checkout_opts *opts,\n>      +          }\n>      +  }\n>      +\n>      +- if (!active_cache_tree)\n>      +-         active_cache_tree = cache_tree();\n>      +-\n>      +  if (!cache_tree_fully_valid(active_cache_tree))\n>      +          cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n>      +\n>      +\n>        ## cache-tree.c ##\n>       @@ cache-tree.c: static int update_one(struct cache_tree *it,\n>\n>      @@ cache-tree.c: static int update_one(struct cache_tree *it,\n>         trace2_region_leave(\"cache_tree\", \"update\", the_repository);\n>         trace_performance_leave(\"cache_tree_update\");\n>         if (i < 0)\n>      +@@ cache-tree.c: static int write_index_as_tree_internal(struct object_id *oid,\n>      +          cache_tree_valid = 0;\n>      +  }\n>      +\n>      +- if (!index_state->cache_tree)\n>      +-         index_state->cache_tree = cache_tree();\n>      +-\n>      +  if (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)\n>      +          return WRITE_TREE_UNMERGED_INDEX;\n>      +\n>      +\n>      + ## sequencer.c ##\n>      +@@ sequencer.c: static int do_recursive_merge(struct repository *r,\n>      +\n>      + static struct object_id *get_cache_tree_oid(struct index_state *istate)\n>      + {\n>      +- if (!istate->cache_tree)\n>      +-         istate->cache_tree = cache_tree();\n>      +-\n>      +  if (!cache_tree_fully_valid(istate->cache_tree))\n>      +          if (cache_tree_update(istate, 0)) {\n>      +                  error(_(\"unable to update cache tree\"));\n>      +\n>      + ## unpack-trees.c ##\n>      +@@ unpack-trees.c: int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>      +          if (!ret) {\n>      +                  if (git_env_bool(\"GIT_TEST_CHECK_CACHE_TREE\", 0))\n>      +                          cache_tree_verify(the_repository, &o->result);\n>      +-                 if (!o->result.cache_tree)\n>      +-                         o->result.cache_tree = cache_tree();\n>      +                  if (!cache_tree_fully_valid(o->result.cache_tree))\n>      +                          cache_tree_update(&o->result,\n>      +                                            WRITE_TREE_SILENT |\n>   -:  ----------- >  2:  1b8b5680094 cache-tree: simplify verify_cache() prototype\n>   2:  84323e04d08 =  3:  314b6b34f75 cache-tree: extract subtree_pos()\n>   3:  31095f9aa0e !  4:  4e688d25f8c fsmonitor: de-duplicate BUG()s around dirty bits\n>      @@ Commit message\n>           cannot simplify it too much. However, the error string is identical in\n>           each, so this simplifies things.\n>\n>      +    Be sure to add one when checking if a position if valid, since the\n>      +    minimum is a bound on the expected size.\n>      +\n>           The end result is that the code is simpler to read while also preserving\n>           these assertions for developers in the FSMonitor space.\n>\n>      @@ fsmonitor.c\n>       - if (pos >= istate->cache_nr)\n>       -         BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n>       -             (uintmax_t)pos, istate->cache_nr);\n>      -+ assert_index_minimum(istate, pos);\n>      ++ assert_index_minimum(istate, pos + 1);\n>\n>         ce = istate->cache[pos];\n>         ce->ce_flags &= ~CE_FSMONITOR_VALID;\n>   4:  a0d89d7a973 !  5:  6373997e05c repository: add repo reference to index_state\n>      @@ Commit message\n>           repository, add a 'repo' pointer to struct index_state that allows\n>           access to this repository.\n>\n>      +    Add a BUG() statement if the repo already has an index, and the index\n>      +    already has a repo, but somehow the index points to a different repo.\n>      +\n>           This will prevent future changes from needing to pass an additional\n>           'struct repository *repo' parameter and instead rely only on the 'struct\n>           index_state *istate' parameter.\n>      @@ repository.c: int repo_read_index(struct repository *repo)\n>       + /* Complete the double-reference */\n>       + if (!repo->index->repo)\n>       +         repo->index->repo = repo;\n>      ++ else if (repo->index->repo != repo)\n>      ++         BUG(\"repo's index should point back at itself\");\n>       +\n>         return read_index_from(repo->index, repo->index_file, repo->gitdir);\n>        }\n>   5:  bc092f5c703 =  6:  9b545d7dbec name-hash: use trace2 regions for init\n>   6:  04d1daf7222 =  7:  554cc7647e6 sparse-checkout: load sparse-checkout patterns\n>   7:  8832ce84623 !  8:  b37181bdec4 test-lib: test_region looks for trace2 regions\n>      @@ t/test-lib-functions.sh: test_subcommand () {\n>       +         shift\n>       + fi\n>       +\n>      -+ grep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n>      ++ grep -e '\"region_enter\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n>       + exitcode=$?\n>       +\n>      -+ if test $exitcode != $expect_exit\n>      ++ if test $exitcode != $expect_exit = 1]\n\nI don't understand this change.  Is it even valid code?  What does it mean?\n\n>       + then\n>       +         return 1\n>       + fi\n>       +\n>      -+ grep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n>      ++ grep -e '\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n>       + exitcode=$?\n>       +\n>      -+ if test $exitcode != $expect_exit\n>      ++ if test $exitcode != $expect_exit = 1]\n\nSame comment.\n\n>       + then\n>       +         return 1\n>       + fi\n>      ++\n>      ++ return 0\n>       +}\n>   8:  984458007ed !  9:  72f925353d3 t1092: test interesting sparse-checkout scenarios\n>      @@ t/t1092-sparse-checkout-compatibility.sh (new)\n>       +         echo a >a &&\n>       +         echo \"after deep\" >e &&\n>       +         echo \"after folder1\" >g &&\n>      ++         echo \"after x\" >z &&\n>       +         mkdir folder1 folder2 deep x &&\n>       +         mkdir deep/deeper1 deep/deeper2 &&\n>       +         mkdir deep/deeper1/deepest &&\n>      @@ t/t1092-sparse-checkout-compatibility.sh (new)\n>       +         echo \"after deepest\" >deep/deeper1/e &&\n>       +         cp a folder1 &&\n>       +         cp a folder2 &&\n>      ++         cp a x &&\n>       +         cp a deep &&\n>       +         cp a deep/deeper1 &&\n>       +         cp a deep/deeper2 &&\n>       +         cp a deep/deeper1/deepest &&\n>      ++         cp -r deep/deeper1/deepest deep/deeper2 &&\n>       +         git add . &&\n>       +         git commit -m \"initial commit\" &&\n>       +         git checkout -b base &&\n>\n\nHaving read the previous rounds, the rest of the range-diff looks good\nto me; I sent out separate comments on the new patch.\n"},{"id":"415094","messageId":"d910be29-ba90-bbe1-fd96-523508b99781@gmail.com","threadId":"55018","inReplyTo":"CABPp-BEsoWs5ZEhS0KTHankzc8eUdmpn0uoF7t1ZtN8b2gwvBA@mail.gmail.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T21:02:22Z","receivedAt":"2021-01-23T21:04:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/23/2021 3:24 PM, Elijah Newren wrote:\n> On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>> -       for (i = 0; i < entries - 1; i++) {\n>> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n>>                 /* path/file always comes after path because of the way\n>>                  * the cache is sorted.  Also path can appear only once,\n>>                  * which means conflicting one would immediately follow.\n>>                  */\n>> -               const struct cache_entry *this_ce = cache[i];\n>> -               const struct cache_entry *next_ce = cache[i + 1];\n>> +               const struct cache_entry *this_ce = istate->cache[i];\n>> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n>>                 const char *this_name = this_ce->name;\n>>                 const char *next_name = next_ce->name;\n>>                 int this_len = ce_namelen(this_ce);\n> Makes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\n> in the commit message; made it easier to read through quickly.  I'm\n> curious if it deserves a comment in the code too, since it does feel\n> slightly unusual.\n\nI would argue that \"i + 1 < N\" is a more natural way to write this,\nbecause we use \"i + 1\" as an index, so we want to ensure the index\nwe are about to use is within range. \"i < N - 1\" is the backwards\nway to write that statement.\n\nThanks,\n-Stolee\n\n\n"},{"id":"415095","messageId":"b4a4df78-fb36-5a27-7ec0-79d752de73ae@gmail.com","threadId":"55018","inReplyTo":"CABPp-BHFiXLOS4Vbm156+JxZw=t2P_Fzf8E3p4u0iwCv28_Rdw@mail.gmail.com","subject":"Re: [PATCH v3 0/9] More index cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T21:05:51Z","receivedAt":"2021-01-23T21:06:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/23/2021 3:29 PM, Elijah Newren wrote:\n> On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> This is based on ds/cache-tree-basics.\n...\n>>      -+ grep -e \"\\\"region_enter\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n>>      ++ grep -e '\"region_enter\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n>>       + exitcode=$?\n>>       +\n>>      -+ if test $exitcode != $expect_exit\n>>      ++ if test $exitcode != $expect_exit = 1]\n> \n> I don't understand this change.  Is it even valid code?  What does it mean?\n> \n>>       + then\n>>       +         return 1\n>>       + fi\n>>       +\n>>      -+ grep -e \"\\\"region_leave\\\".*\\\"category\\\":\\\"$1\\\",\\\"label\\\":\\\"$2\\\"\" \"$3\"\n>>      ++ grep -e '\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n>>       + exitcode=$?\n>>       +\n>>      -+ if test $exitcode != $expect_exit\n>>      ++ if test $exitcode != $expect_exit = 1]\n> \n> Same comment\n\nWow. I am sorry this snuck in. It's an artifact of what I was trying [1]\nin response to Junio's comments, but then did not completely undo. I'm\nsurprised this ran without error. Will fix.\n\n[1] https://lore.kernel.org/git/8406512b-3d9f-e899-24fd-8a09c4af3569@gmail.com/\n"},{"id":"415096","messageId":"096e2969-60d2-3ebe-3855-a9c954ebd07e@gmail.com","threadId":"55018","inReplyTo":"b37181bdec43cfc798740f2bdd19f6d2482beb26.1611431900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 8/9] test-lib: test_region looks for trace2 regions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T21:07:08Z","receivedAt":"2021-01-23T21:08:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/23/2021 2:58 PM, Derrick Stolee via GitGitGadget wrote:\n...\n> +\tif test $exitcode != $expect_exit = 1]\n...\n> +\tif test $exitcode != $expect_exit = 1]\nAs Elijah pointed out, these lines are bogus. I'm not sure how\nthey passed the tests without failure, but here is a replacement\nfor this patch:\n\n--- >8 ---\n\n\nFrom ff15d509b89edd4830d85d53cea3079a6b0c1c08 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee <dstolee@microsoft.com>\nDate: Mon, 11 Jan 2021 08:53:09 -0500\nSubject: [PATCH 8/9] test-lib: test_region looks for trace2 regions\n\nMost test cases can verify Git's behavior using input/output\nexpectations or changes to the .git directory. However, sometimes we\nwant to check that Git did or did not run a certain section of code.\nThis is particularly important for performance-only features that we\nwant to ensure have been enabled in certain cases.\n\nAdd a new 'test_region' function that checks if a trace2 region was\nentered and left in a given trace2 event log.\n\nThere is one existing test (t0500-progress-display.sh) that performs\nthis check already, so use the helper function instead. Note that this\nchanges the expectations slightly. The old test (incorrectly) used two\npatterns for the 'grep' invocation, but this performs an OR of the\npatterns, not an AND. This means that as long as one region_enter event\nwas logged, the test would succeed, even if it was not due to the\nprogress category.\n\nMore uses will be added in a later change.\n\nt6423-merge-rename-directories.sh also greps for region_enter lines, but\nit verifies the number of such lines, which is not the same as an\nexistence check.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t0500-progress-display.sh |  3 +--\n t/test-lib-functions.sh     | 42 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 1ed1df351c..84cce345e7 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '\n \t\t\"Working hard\" <in 2>stderr &&\n \n \t# t0212/parse_events.perl intentionally omits regions and data.\n-\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n-\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\ttest_region progress \"Working hard\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a..9fc4cf8476 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1655,3 +1655,45 @@ test_subcommand () {\n \t\tgrep \"\\[$expr\\]\"\n \tfi\n }\n+\n+# Check that the given command was invoked as part of the\n+# trace2-format trace on stdin.\n+#\n+#\ttest_region [!] <category> <label> git <command> <args>...\n+#\n+# For example, to look for trace2_region_enter(\"index\", \"do_read_index\", repo)\n+# in an invocation of \"git checkout HEAD~1\", run\n+#\n+#\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" GIT_TRACE2_EVENT_NESTING=10 \\\n+#\t\tgit checkout HEAD~1 &&\n+#\ttest_region index do_read_index <trace.txt\n+#\n+# If the first parameter passed is !, this instead checks that\n+# the given region was not entered.\n+#\n+test_region () {\n+\tlocal expect_exit=0\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\texpect_exit=1\n+\t\tshift\n+\tfi\n+\n+\tgrep -e\t'\"region_enter\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\tgrep -e\t'\"region_leave\".*\"category\":\"'\"$1\"'\",\"label\":\"'\"$2\"\\\" \"$3\"\n+\texitcode=$?\n+\n+\tif test $exitcode != $expect_exit\n+\tthen\n+\t\treturn 1\n+\tfi\n+\n+\treturn 0\n+}\n-- \n2.30.0\n\n"},{"id":"415097","messageId":"xmqqy2gj2wc0.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"CABPp-BEsoWs5ZEhS0KTHankzc8eUdmpn0uoF7t1ZtN8b2gwvBA@mail.gmail.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-23T21:10:39Z","receivedAt":"2021-01-23T21:11:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> -       for (i = 0; i < entries - 1; i++) {\n>> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n>>                 /* path/file always comes after path because of the way\n>>                  * the cache is sorted.  Also path can appear only once,\n>>                  * which means conflicting one would immediately follow.\n>>                  */\n>> -               const struct cache_entry *this_ce = cache[i];\n>> -               const struct cache_entry *next_ce = cache[i + 1];\n>> +               const struct cache_entry *this_ce = istate->cache[i];\n>> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n>\n> Makes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\n> in the commit message; made it easier to read through quickly.  I'm\n> curious if it deserves a comment in the code too, since it does feel\n> slightly unusual.\n\nI think this is entirely my fault.\n\nIt probably reads more natural to start from 1 and interate through\nto the end of the array, comparing the current one with the previous\nentry, i.e.\n\n\tfor (i = 1; i < istate->cache_nr; i++) {\n        \tprev = cache[i - 1];\n\t\tthis = cache[i];\n                compare(prev, this);\n\n"},{"id":"415098","messageId":"CABPp-BHGFU_2t7gRgdzTgf33y-_9LkLVZtp5RCjfty2mVme_yA@mail.gmail.com","threadId":"55018","inReplyTo":"d910be29-ba90-bbe1-fd96-523508b99781@gmail.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-23T21:10:41Z","receivedAt":"2021-01-23T21:11:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 23, 2021 at 1:02 PM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 1/23/2021 3:24 PM, Elijah Newren wrote:\n> > On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> >> -       for (i = 0; i < entries - 1; i++) {\n> >> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n> >>                 /* path/file always comes after path because of the way\n> >>                  * the cache is sorted.  Also path can appear only once,\n> >>                  * which means conflicting one would immediately follow.\n> >>                  */\n> >> -               const struct cache_entry *this_ce = cache[i];\n> >> -               const struct cache_entry *next_ce = cache[i + 1];\n> >> +               const struct cache_entry *this_ce = istate->cache[i];\n> >> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n> >>                 const char *this_name = this_ce->name;\n> >>                 const char *next_name = next_ce->name;\n> >>                 int this_len = ce_namelen(this_ce);\n> > Makes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\n> > in the commit message; made it easier to read through quickly.  I'm\n> > curious if it deserves a comment in the code too, since it does feel\n> > slightly unusual.\n>\n> I would argue that \"i + 1 < N\" is a more natural way to write this,\n> because we use \"i + 1\" as an index, so we want to ensure the index\n> we are about to use is within range. \"i < N - 1\" is the backwards\n> way to write that statement.\n\nOh, right, I think I was reading too quickly and assuming one thing in\nmy head (about what the code was going to do), and forgetting that\nassumption when I got to the actual code.  Sorry about that; I agree\nwith you, so ignore my previous comment.\n"},{"id":"415099","messageId":"51dc4a80-1bea-a028-ccd1-ccae968a82a3@gmail.com","threadId":"55018","inReplyTo":"xmqqy2gj2wc0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-23T21:14:36Z","receivedAt":"2021-01-23T21:15:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/23/2021 4:10 PM, Junio C Hamano wrote:\n> Elijah Newren <newren@gmail.com> writes:\n> \n>>> -       for (i = 0; i < entries - 1; i++) {\n>>> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n>>>                 /* path/file always comes after path because of the way\n>>>                  * the cache is sorted.  Also path can appear only once,\n>>>                  * which means conflicting one would immediately follow.\n>>>                  */\n>>> -               const struct cache_entry *this_ce = cache[i];\n>>> -               const struct cache_entry *next_ce = cache[i + 1];\n>>> +               const struct cache_entry *this_ce = istate->cache[i];\n>>> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n>>\n>> Makes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\n>> in the commit message; made it easier to read through quickly.  I'm\n>> curious if it deserves a comment in the code too, since it does feel\n>> slightly unusual.\n> \n> I think this is entirely my fault.\n> \n> It probably reads more natural to start from 1 and interate through\n> to the end of the array, comparing the current one with the previous\n> entry, i.e.\n> \n> \tfor (i = 1; i < istate->cache_nr; i++) {\n>         \tprev = cache[i - 1];\n> \t\tthis = cache[i];\n>                 compare(prev, this);\n\nThis would be another natural way to make the loop extremely clear.\n\nIt's a bigger diff, changing the names of 'this' and 'next', but\nperhaps worthwhile.\n\nThanks,\n-Stolee\n\n"},{"id":"415103","messageId":"xmqqh7n72uwt.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"d910be29-ba90-bbe1-fd96-523508b99781@gmail.com","subject":"Re: [PATCH v3 2/9] cache-tree: simplify verify_cache() prototype","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-23T21:41:22Z","receivedAt":"2021-01-23T21:42:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 1/23/2021 3:24 PM, Elijah Newren wrote:\n>> On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n>> <gitgitgadget@gmail.com> wrote:\n>>> -       for (i = 0; i < entries - 1; i++) {\n>>> +       for (i = 0; i + 1 < istate->cache_nr; i++) {\n>>>                 /* path/file always comes after path because of the way\n>>>                  * the cache is sorted.  Also path can appear only once,\n>>>                  * which means conflicting one would immediately follow.\n>>>                  */\n>>> -               const struct cache_entry *this_ce = cache[i];\n>>> -               const struct cache_entry *next_ce = cache[i + 1];\n>>> +               const struct cache_entry *this_ce = istate->cache[i];\n>>> +               const struct cache_entry *next_ce = istate->cache[i + 1];\n>>>                 const char *this_name = this_ce->name;\n>>>                 const char *next_name = next_ce->name;\n>>>                 int this_len = ce_namelen(this_ce);\n>> Makes sense.  Thanks for explaining the i + 1 < istate->cache_nr bit\n>> in the commit message; made it easier to read through quickly.  I'm\n>> curious if it deserves a comment in the code too, since it does feel\n>> slightly unusual.\n>\n> I would argue that \"i + 1 < N\" is a more natural way to write this,\n> because we use \"i + 1\" as an index, so we want to ensure the index\n> we are about to use is within range. \"i < N - 1\" is the backwards\n> way to write that statement.\n\nOur mails have crossed, I guess.  Comparing i+1 and N is also good.\n\n\n"},{"id":"415104","messageId":"xmqqczxv2uug.fsf@gitster.c.googlers.com","threadId":"55018","inReplyTo":"b4a4df78-fb36-5a27-7ec0-79d752de73ae@gmail.com","subject":"Re: [PATCH v3 0/9] More index cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-23T21:42:47Z","receivedAt":"2021-01-23T21:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>>>      -+ if test $exitcode != $expect_exit\n>>>      ++ if test $exitcode != $expect_exit = 1]\n>> \n>> Same comment\n>\n> Wow. I am sorry this snuck in. It's an artifact of what I was trying [1]\n> in response to Junio's comments, but then did not completely undo.\n\nSorry about that.\n"},{"id":"415322","messageId":"CABPp-BF8rU5WqGrySFM2HTc1Ro5hCLiFMwesu7W9JbNsAtFCTw@mail.gmail.com","threadId":"55018","inReplyTo":"72f925353d3f3992ab6a98df2b0752c019d11338.1611431900.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 9/9] t1092: test interesting sparse-checkout scenarios","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-25T18:45:47Z","receivedAt":"2021-01-26T20:52:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> These also document some behaviors that differ from a full checkout, and\n> possibly in a way that is not intended.\n>\n> The test is designed to be run with \"--run=1,X\" where 'X' is an\n\nRandom sidenote: Why not suggest \"--run=setup,X\" instead?  Then if you\never add additional \"setup\" steps, it'd run all of them.\n\n(See https://lore.kernel.org/git/0355c888828aeec331a6b99a26d8aa04cff59859.1602980628.git.gitgitgadget@gmail.com/)\n\n> interesting test case. Each test uses 'init_repos' to reset the full and\n> sparse copies of the initial-repo that is created by the first test\n> case. This also makes it possible to have test cases leave the working\n> directory or index in unusual states without disturbing later cases.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  t/t1092-sparse-checkout-compatibility.sh | 301 +++++++++++++++++++++++\n>  1 file changed, 301 insertions(+)\n>  create mode 100755 t/t1092-sparse-checkout-compatibility.sh\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> new file mode 100755\n> index 00000000000..8cd3e5a8d22\n> --- /dev/null\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -0,0 +1,301 @@\n> +#!/bin/sh\n> +\n> +test_description='compare full workdir to sparse workdir'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +       git init initial-repo &&\n> +       (\n> +               cd initial-repo &&\n> +               echo a >a &&\n> +               echo \"after deep\" >e &&\n> +               echo \"after folder1\" >g &&\n> +               echo \"after x\" >z &&\n> +               mkdir folder1 folder2 deep x &&\n> +               mkdir deep/deeper1 deep/deeper2 &&\n> +               mkdir deep/deeper1/deepest &&\n> +               echo \"after deeper1\" >deep/e &&\n> +               echo \"after deepest\" >deep/deeper1/e &&\n> +               cp a folder1 &&\n> +               cp a folder2 &&\n> +               cp a x &&\n> +               cp a deep &&\n> +               cp a deep/deeper1 &&\n> +               cp a deep/deeper2 &&\n> +               cp a deep/deeper1/deepest &&\n> +               cp -r deep/deeper1/deepest deep/deeper2 &&\n> +               git add . &&\n> +               git commit -m \"initial commit\" &&\n> +               git checkout -b base &&\n> +               for dir in folder1 folder2 deep\n> +               do\n> +                       git checkout -b update-$dir &&\n> +                       echo \"updated $dir\" >$dir/a &&\n> +                       git commit -a -m \"update $dir\" || return 1\n> +               done &&\n> +\n> +               git checkout -b rename-base base &&\n> +               echo >folder1/larger-content <<-\\EOF &&\n> +               matching\n> +               lines\n> +               help\n> +               inexact\n> +               renames\n> +               EOF\n> +               cp folder1/larger-content folder2/ &&\n> +               cp folder1/larger-content deep/deeper1/ &&\n> +               git add . &&\n> +               git commit -m \"add interesting rename content\" &&\n> +\n> +               git checkout -b rename-out-to-out rename-base &&\n> +               mv folder1/a folder2/b &&\n> +               mv folder1/larger-content folder2/edited-content &&\n> +               echo >>folder2/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename folder1/... to folder2/...\" &&\n> +\n> +               git checkout -b rename-out-to-in rename-base &&\n> +               mv folder1/a deep/deeper1/b &&\n> +               mv folder1/larger-content deep/deeper1/edited-content &&\n> +               echo >>deep/deeper1/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename folder1/... to deep/deeper1/...\" &&\n> +\n> +               git checkout -b rename-in-to-out rename-base &&\n> +               mv deep/deeper1/a folder1/b &&\n> +               mv deep/deeper1/larger-content folder1/edited-content &&\n> +               echo >>folder1/edited-content &&\n> +               git add . &&\n> +               git commit -m \"rename deep/deeper1/... to folder1/...\" &&\n> +\n> +               git checkout -b deepest base &&\n> +               echo \"updated deepest\" >deep/deeper1/deepest/a &&\n> +               git commit -a -m \"update deepest\" &&\n> +\n> +               git checkout -f base &&\n> +               git reset --hard\n> +       )\n> +'\n> +\n> +init_repos () {\n> +       rm -rf full-checkout sparse-checkout sparse-index &&\n> +\n> +       # create repos in initial state\n> +       cp -r initial-repo full-checkout &&\n> +       git -C full-checkout reset --hard &&\n> +\n> +       cp -r initial-repo sparse-checkout &&\n> +       git -C sparse-checkout reset --hard &&\n> +       git -C sparse-checkout sparse-checkout init --cone &&\n> +\n> +       # initialize sparse-checkout definitions\n> +       git -C sparse-checkout sparse-checkout set deep\n> +}\n> +\n> +run_on_sparse () {\n> +       (\n> +               cd sparse-checkout &&\n> +               $* >../sparse-checkout-out 2>../sparse-checkout-err\n> +       )\n> +}\n> +\n> +run_on_all () {\n> +       (\n> +               cd full-checkout &&\n> +               $* >../full-checkout-out 2>../full-checkout-err\n> +       ) &&\n> +       run_on_sparse $*\n> +}\n> +\n> +test_all_match () {\n> +       run_on_all $* &&\n> +       test_cmp full-checkout-out sparse-checkout-out &&\n> +       test_cmp full-checkout-err sparse-checkout-err\n> +}\n> +\n> +test_expect_success 'status with options' '\n> +       init_repos &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       run_on_all \"touch README.md\" &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       test_all_match git add README.md &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git status --porcelain=v2 -z -u &&\n> +       test_all_match git status --porcelain=v2 -uno\n> +'\n> +\n> +test_expect_success 'add, commit, checkout' '\n> +       init_repos &&\n> +\n> +       write_script edit-contents <<-\\EOF &&\n> +       echo text >>$1\n> +       EOF\n> +       run_on_all \"../edit-contents README.md\" &&\n> +\n> +       test_all_match git add README.md &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Add README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout - &&\n> +\n> +       run_on_all \"../edit-contents README.md\" &&\n> +\n> +       test_all_match git add -A &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"Extend README.md\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout - &&\n> +\n> +       run_on_all \"../edit-contents deep/newfile\" &&\n> +\n> +       test_all_match git status --porcelain=v2 -uno &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git add . &&\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git commit -m \"add deep/newfile\" &&\n> +\n> +       test_all_match git checkout HEAD~1 &&\n> +       test_all_match git checkout -\n> +'\n> +\n> +test_expect_success 'checkout and reset --hard' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout update-folder1 &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git checkout update-deep &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git checkout -b reset-test &&\n> +       test_all_match git reset --hard deepest &&\n> +       test_all_match git reset --hard update-folder1 &&\n> +       test_all_match git reset --hard update-folder2\n> +'\n> +\n> +test_expect_success 'diff --staged' '\n> +       init_repos &&\n> +\n> +       write_script edit-contents <<-\\EOF &&\n> +       echo text >>README.md\n> +       EOF\n> +       run_on_all \"../edit-contents\" &&\n> +\n> +       test_all_match git diff &&\n> +       test_all_match git diff --staged &&\n> +       test_all_match git add README.md &&\n> +       test_all_match git diff &&\n> +       test_all_match git diff --staged\n> +'\n> +\n> +test_expect_success 'diff with renames' '\n> +       init_repos &&\n> +\n> +       for branch in rename-out-to-out rename-out-to-in rename-in-to-out\n> +       do\n> +               test_all_match git checkout rename-base &&\n> +               test_all_match git checkout $branch -- .&&\n> +               test_all_match git diff --staged --no-renames &&\n> +               test_all_match git diff --staged --find-renames || return 1\n> +       done\n> +'\n> +\n> +test_expect_success 'log with pathspec outside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git log -- a &&\n> +       test_all_match git log -- folder1/a &&\n> +       test_all_match git log -- folder2/a &&\n> +       test_all_match git log -- deep/a &&\n> +       test_all_match git log -- deep/deeper1/a &&\n> +       test_all_match git log -- deep/deeper1/deepest/a &&\n> +\n> +       test_all_match git checkout update-folder1 &&\n> +       test_all_match git log -- folder1/a\n> +'\n> +\n> +test_expect_success 'blame with pathspec inside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git blame a &&\n> +       test_all_match git blame deep/a &&\n> +       test_all_match git blame deep/deeper1/a &&\n> +       test_all_match git blame deep/deeper1/deepest/a\n> +'\n> +\n> +# TODO: blame currently does not support blaming files outside of the\n> +# sparse definition. It complains that the file doesn't exist locally.\n> +test_expect_failure 'blame with pathspec outside sparse definition' '\n> +       init_repos &&\n> +\n> +       test_all_match git blame folder1/a &&\n> +       test_all_match git blame folder2/a &&\n> +       test_all_match git blame deep/deeper2/a &&\n> +       test_all_match git blame deep/deeper2/deepest/a\n> +'\n> +\n> +# TODO: reset currently does not behave as expected when in a\n> +# sparse-checkout.\n> +test_expect_failure 'checkout and reset (mixed)' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout -b reset-test update-deep &&\n> +       test_all_match git reset deepest &&\n> +       test_all_match git reset update-folder1 &&\n> +       test_all_match git reset update-folder2\n> +'\n> +\n> +test_expect_success 'merge' '\n> +       init_repos &&\n> +\n> +       test_all_match git checkout -b merge update-deep &&\n> +       test_all_match git merge -m \"folder1\" update-folder1 &&\n> +       test_all_match git rev-parse HEAD^{tree} &&\n> +       test_all_match git merge -m \"folder2\" update-folder2 &&\n> +       test_all_match git rev-parse HEAD^{tree}\n> +'\n> +\n> +test_expect_success 'merge with outside renames' '\n> +       init_repos &&\n> +\n> +       for type in out-to-out out-to-in in-to-out\n> +       do\n> +               test_all_match git reset --hard &&\n> +               test_all_match git checkout -f -b merge-$type update-deep &&\n> +               test_all_match git merge -m \"$type\" rename-$type &&\n> +               test_all_match git rev-parse HEAD^{tree} || return 1\n> +       done\n> +'\n> +\n> +test_expect_success 'clean' '\n> +       init_repos &&\n> +\n> +       echo bogus >>.gitignore &&\n> +       run_on_all cp ../.gitignore . &&\n> +       test_all_match git add .gitignore &&\n> +       test_all_match git commit -m ignore-bogus-files &&\n> +\n> +       run_on_sparse mkdir folder1 &&\n> +       run_on_all touch folder1/bogus &&\n> +\n> +       test_all_match git status --porcelain=v2 &&\n> +       test_all_match git clean -f &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git clean -xf &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_all_match git clean -xdf &&\n> +       test_all_match git status --porcelain=v2 &&\n> +\n> +       test_path_is_dir sparse-checkout/folder1\n> +'\n> +\n> +test_done\n> --\n> gitgitgadget\n"}]}