More index cleanups

From: Derrick Stolee <[email protected]>

Make the method safer by allocating a cache_tree member for the given index_state if it is not already present.

Also drop local variables that are used exactly once and can be found directly from the 'istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache-tree.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

cache-tree.c +0 -0

index 3f1a8d4f1b7..c1e49901c17 100644

cache-tree.c +0 -1

        trace2_region_leave("cache_tree", "update", the_repository);
        trace_performance_leave("cache_tree_update");
        if (i < 0)
--

gitgitgadget

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget <[email protected]> wrote:...

cache-tree.c static int update_one(struct cache_tree *it, · line 447

Quoted 3 lines

cache-tree.c +3 -0

+
+       if (!istate->cache_tree)
+               istate->cache_tree = cache_tree();

This is the only substantive change. It seems fairly innocuous, but it makes me wonder the reasoning...I don't know/remember enough about cache_tree handling to know when this would or wouldn't have already been allocated. It seems that this would have had to segfault below if istate->cache_tree were ever NULL, and I don't see you mentioning any bug you are fixing, so I presume this means you are going to be adding new codepaths somewhere that cause this function to be reached under different circumstances than previously had been and you need it to be more safe for those. Is that correct? Or is it just an abundance of caution thing that you're adding? If the latter, any reason you chose to allocate one rather than assume it's a violation of design invariants and BUG() instead? (Perhaps the commit message could add a sentence about the rationale for the extra safety?)

From: Derrick Stolee <[email protected]>

This method will be helpful to use outside of cache-tree.c in a later feature. The implementation is subtle due to subtree_name_cmp() sorting by length and then lexicographically.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache-tree.c | 6 +++---
 cache-tree.h | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

cache-tree.c +3 -3

index c1e49901c17..2b130dd5e19 100644
@@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,
        return memcmp(one, two, onelen);
 }
 
-static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)
+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen)
 {
        struct cache_tree_sub **down = it->down;
        int lo, hi;

@@ -72,7 +72,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it,
                                           int create)
 {
        struct cache_tree_sub *down;
-       int pos = subtree_pos(it, path, pathlen);
+       int pos = cache_tree_subtree_pos(it, path, pathlen);
        if (0 <= pos)
                return it->down[pos];
        if (!create)

@@ -123,7 +123,7 @@ static int do_invalidate_path(struct cache_tree *it, const char *path)
        it->entry_count = -1;
        if (!*slash) {
                int pos;
-               pos = subtree_pos(it, path, namelen);
+               pos = cache_tree_subtree_pos(it, path, namelen);
                if (0 <= pos) {
                        cache_tree_free(&it->down[pos]->cache_tree);
                        free(it->down[pos]);

cache-tree.h +2 -1

index 639bfa5340e..8efeccebfc9 100644
@@ -27,6 +27,8 @@ void cache_tree_free(struct cache_tree **);
 void cache_tree_invalidate_path(struct index_state *, const char *);
 struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);
 
+int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);
+
 void cache_tree_write(struct strbuf *, struct cache_tree *root);
 struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
 
--

gitgitgadget

From: Derrick Stolee <[email protected]>

The index has an fsmonitor_dirty bitmap that records which index entries are "dirty" based on the response from the FSMonitor. If this bitmap ever grows larger than the index, then there was an error in how it was constructed, and it was probably a developer's bug.

There are several BUG() statements that are very similar, so replace these uses with a simpler assert_index_minimum(). Since there is one caller that uses a custom 'pos' value instead of the bit_size member, we cannot simplify it too much. However, the error string is identical in each, so this simplifies things.

The end result is that the code is simpler to read while also preserving these assertions for developers in the FSMonitor space.

Signed-off-by Derrick Stolee <[email protected]>

---
 fsmonitor.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

fsmonitor.c +13 -15

index ca031c3abb8..52a50a9545a 100644
@@ -13,14 +13,19 @@
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 
+static void assert_index_minimum(struct index_state *istate, size_t pos)
+{
+       if (pos > istate->cache_nr)
+               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
+                   (uintmax_t)pos, istate->cache_nr);
+}
+
 static void fsmonitor_ewah_callback(size_t pos, void *is)
 {
        struct index_state *istate = (struct index_state *)is;
        struct cache_entry *ce;
 
-       if (pos >= istate->cache_nr)
-               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
-                   (uintmax_t)pos, istate->cache_nr);
+       assert_index_minimum(istate, pos);
 
        ce = istate->cache[pos];
        ce->ce_flags &= ~CE_FSMONITOR_VALID;

@@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
        }
        istate->fsmonitor_dirty = fsmonitor_dirty;
 
-       if (!istate->split_index &&
-           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+       if (!istate->split_index)
+               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
        trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
        return 0;

@@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
        uint32_t ewah_size = 0;
        int fixup = 0;
 
-       if (!istate->split_index &&
-           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+       if (!istate->split_index)
+               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
        put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
        strbuf_add(sb, &hdr_version, sizeof(uint32_t));

@@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)
                        }
 
                        /* Mark all previously saved entries as dirty */
-                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-                               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-                                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+                       assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
                        ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
                        refresh_fsmonitor(istate);
--

gitgitgadget

From: Derrick Stolee <[email protected]>

The index has an fsmonitor_dirty bitmap that records which index entries are "dirty" based on the response from the FSMonitor. If this bitmap ever grows larger than the index, then there was an error in how it was constructed, and it was probably a developer's bug.

Quoted 7 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

The index has an fsmonitor_dirty bitmap that records which index entries are "dirty" based on the response from the FSMonitor. If this bitmap ever grows larger than the index, then there was an error in how it was constructed, and it was probably a developer's bug.

Curious: some of the tests were >=, some were > (not >=). Now that they're shared in a function they are all ">".

It's pretty clear that for size-based ones, greater-than is the right test, but for position ones, isn't it still greater-or-equal? So perhaps the calls that pass an actual position should add 1...

From: Derrick Stolee <[email protected]>

It will be helpful to add behavior to index opertations that might

trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct index_state that allows access to this repository.

This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct index_state *istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache.h      | 1 +
 repository.c | 4 ++++
 2 files changed, 5 insertions(+)

cache.h +1 -0

index 71097657489..f9c7a603841 100644
@@ -328,6 +328,7 @@ struct index_state {
        struct ewah_bitmap *fsmonitor_dirty;
        struct mem_pool *ce_mem_pool;
        struct progress *progress;
+       struct repository *repo;
 };
 
 /* Name hashing */

repository.c +4 -1

index a4174ddb062..67a4c1da2d9 100644
@@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)
        if (!repo->index)
                repo->index = xcalloc(1, sizeof(*repo->index));
 
+       /* Complete the double-reference */
+       if (!repo->index->repo)
+               repo->index->repo = repo;
+
        return read_index_from(repo->index, repo->index_file, repo->gitdir);
 }
 
--

gitgitgadget

Quoted 4 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

It will be helpful to add behavior to index opertations that might

s/opertations/operations/

Quoted 41 lines

trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct index_state that allows access to this repository.

This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct index_state *istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache.h      | 1 +
 repository.c | 4 ++++
 2 files changed, 5 insertions(+)

cache.h +1 -0

index 71097657489..f9c7a603841 100644
@@ -328,6 +328,7 @@ struct index_state {
        struct ewah_bitmap *fsmonitor_dirty;
        struct mem_pool *ce_mem_pool;
        struct progress *progress;
+       struct repository *repo;
 };
 
 /* Name hashing */

repository.c +4 -0

index a4174ddb062..67a4c1da2d9 100644
@@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)
        if (!repo->index)
                repo->index = xcalloc(1, sizeof(*repo->index));
 
+       /* Complete the double-reference */
+       if (!repo->index->repo)
+               repo->index->repo = repo;
+
        return read_index_from(repo->index, repo->index_file, repo->gitdir);
 }
 

Quoted 2 lines

--
gitgitgadget

Since we have repo->index and we have index->repo, which are intended to be circular...what if they aren't? Do we want or need to add assertions anywhere that repo == repo->index->repo or that index == index->repo->index ?

My initial implementations of --remerge-diff[1] played around with creating a second repo, with a different primary object store but everything else the same. The index for the two repository objects was thus the same, and thus clearly would have violated this assumed invariant for one of the two repos. I discarded that initial implementation (which I didn't quite have working) because I discovered tmp-objdir.h and was able to add some tmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions that merely altered the existing repo's primary object store, but I'm curious if there might be other cases of folks doing stuff that might have weird failures with this new invariant.

It's entirely possible that --remerge-diff was just so different, and I was so unfamiliar with repo objects (and still kind of am) that I was just doing weird stuff no one has done before, so perhaps no additional checks are needed -- I'm just throwing my gut question out there as food for thought.

[1] I have not yet submitted --remerge-diff to the list; you haven't missed anything. I'm waiting for merge-ort to be submitted, reviewed, and merged first. It's the remerge-diff branch in my fork on GitHub if anyone is curious, though.

Quoted 8 lines · Elijah Newren

Quoted 4 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

It will be helpful to add behavior to index opertations that might

s/opertations/operations/

Thanks.

Quoted 41 lines

Quoted 41 lines

trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct index_state that allows access to this repository.

This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct index_state *istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache.h      | 1 +
 repository.c | 4 ++++
 2 files changed, 5 insertions(+)

cache.h +1 -0

index 71097657489..f9c7a603841 100644
@@ -328,6 +328,7 @@ struct index_state {
        struct ewah_bitmap *fsmonitor_dirty;
        struct mem_pool *ce_mem_pool;
        struct progress *progress;
+       struct repository *repo;
 };
 
 /* Name hashing */

repository.c +4 -0

index a4174ddb062..67a4c1da2d9 100644
@@ -264,6 +264,10 @@ int repo_read_index(struct repository *repo)
        if (!repo->index)
                repo->index = xcalloc(1, sizeof(*repo->index));
 
+       /* Complete the double-reference */
+       if (!repo->index->repo)
+               repo->index->repo = repo;
+
        return read_index_from(repo->index, repo->index_file, repo->gitdir);
 }
 

Quoted 7 lines

Quoted 2 lines

--
gitgitgadget

Since we have repo->index and we have index->repo, which are intended to be circular...what if they aren't? Do we want or need to add assertions anywhere that repo == repo->index->repo or that index == index->repo->index ?

Here, we are pairing them together and the loop is complete. I don't view that as a permanent thing. This only initializes istate->repo when we are parsing an index from a file, but not when we create one in memory.

I imagine it will be likely in some cases to have multiple index_state instances for a single repository. However, having the pointer "this index belongs to this repository" seems helpful (to me).

Quoted 11 lines

My initial implementations of --remerge-diff[1] played around with creating a second repo, with a different primary object store but everything else the same. The index for the two repository objects was thus the same, and thus clearly would have violated this assumed invariant for one of the two repos. I discarded that initial implementation (which I didn't quite have working) because I discovered tmp-objdir.h and was able to add some tmp_objdir_make_primary() and tmp_objdir_remove_as_primary() functions that merely altered the existing repo's primary object store, but I'm curious if there might be other cases of folks doing stuff that might have weird failures with this new invariant.

This is an interesting concept, and definitely violates my expectations that an index belongs to only one repository. I'd need to know more about why this was a good design decision before being convinced that the relationship should not be many-to-one (index-to-repo).

Quoted 10 lines

It's entirely possible that --remerge-diff was just so different, and I was so unfamiliar with repo objects (and still kind of am) that I was just doing weird stuff no one has done before, so perhaps no additional checks are needed -- I'm just throwing my gut question out there as food for thought.

[1] I have not yet submitted --remerge-diff to the list; you haven't missed anything. I'm waiting for merge-ort to be submitted, reviewed, and merged first. It's the remerge-diff branch in my fork on GitHub if anyone is curious, though.

I'm interested in what others might say about this idea. I'd be able to do most of what I want to do without this patch, but it just gets a lot messier. (istate->repo is used in the very next patch in a way that would be less clean without it.)

From: Derrick Stolee <[email protected]>

The lazy_init_name_hash() populates a hashset with all filenames and another with all directories represented in the index. This is run only if we need to use the hashsets to check for existence or case-folding renames.

Place trace2 regions where there is already a performance trace.

Signed-off-by Derrick Stolee <[email protected]>

---
 name-hash.c | 3 +++
 1 file changed, 3 insertions(+)

name-hash.c +3 -1

index 5d3c7b12c18..4e03fac9bb1 100644
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "thread-utils.h"
+#include "trace2.h"
 
 struct dir_entry {
        struct hashmap_entry ent;

@@ -577,6 +578,7 @@ static void lazy_init_name_hash(struct index_state *istate)
        if (istate->name_hash_initialized)
                return;
        trace_performance_enter();
+       trace2_region_enter("index", "name-hash-init", istate->repo);
        hashmap_init(&istate->name_hash, cache_entry_cmp, NULL, istate->cache_nr);
        hashmap_init(&istate->dir_hash, dir_entry_cmp, NULL, istate->cache_nr);
 

@@ -597,6 +599,7 @@ static void lazy_init_name_hash(struct index_state *istate)
        }
 
        istate->name_hash_initialized = 1;
+       trace2_region_leave("index", "name-hash-init", istate->repo);
        trace_performance_leave("initialize name hash");
 }
 
--

gitgitgadget

From: Derrick Stolee <[email protected]>

A future feature will want to load the sparse-checkout patterns into a pattern_list, but the current mechanism to do so is a bit complicated. This is made difficult due to needing to find the sparse-checkout file in different ways throughout the codebase.

The logic implemented in the new get_sparse_checkout_patterns() was duplicated in populate_from_existing_patterns() in unpack-trees.c. Use the new method instead, keeping the logic around handling the struct unpack_trees_options.

The callers to get_sparse_checkout_filename() in builtin/sparse-checkout.c manipulate the sparse-checkout file directly, so it is not appropriate to replace logic in that file with get_sparse_checkout_patterns().

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c |  5 -----
 dir.c                     | 17 +++++++++++++++++
 dir.h                     |  2 ++
 unpack-trees.c            |  6 +-----
 4 files changed, 20 insertions(+), 10 deletions(-)

builtin/sparse-checkout.c +0 -5

index e3140db2a0a..2306a9ad98e 100644
@@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {
        NULL
 };
 
-static char *get_sparse_checkout_filename(void)
-{
-       return git_pathdup("info/sparse-checkout");
-}
-
 static void write_patterns_to_file(FILE *fp, struct pattern_list *pl)
 {
        int i;

dir.c +17 -0

index d637461da5c..d153a63bbd1 100644
@@ -2998,6 +2998,23 @@ void setup_standard_excludes(struct dir_struct *dir)
        }
 }
 
+char *get_sparse_checkout_filename(void)
+{
+       return git_pathdup("info/sparse-checkout");
+}
+
+int get_sparse_checkout_patterns(struct pattern_list *pl)
+{
+       int res;
+       char *sparse_filename = get_sparse_checkout_filename();
+
+       pl->use_cone_patterns = core_sparse_checkout_cone;
+       res = add_patterns_from_file_to_list(sparse_filename, "", 0, pl, NULL);
+
+       free(sparse_filename);
+       return res;
+}
+
 int remove_path(const char *name)
 {
        char *slash;

dir.h +2 -0

index a3c40dec516..facfae47402 100644
@@ -448,6 +448,8 @@ int is_empty_dir(const char *dir);
 
 void setup_standard_excludes(struct dir_struct *dir);
 
+char *get_sparse_checkout_filename(void);
+int get_sparse_checkout_patterns(struct pattern_list *pl);
 
 /* Constants for remove_dir_recursively: */
 

unpack-trees.c +1 -6

index af6e9b9c2fd..837b8bb42fb 100644
@@ -1549,14 +1549,10 @@ static void mark_new_skip_worktree(struct pattern_list *pl,
 static void populate_from_existing_patterns(struct unpack_trees_options *o,
                                            struct pattern_list *pl)
 {
-       char *sparse = git_pathdup("info/sparse-checkout");
-
-       pl->use_cone_patterns = core_sparse_checkout_cone;
-       if (add_patterns_from_file_to_list(sparse, "", 0, pl, NULL) < 0)
+       if (get_sparse_checkout_patterns(pl) < 0)
                o->skip_sparse_checkout = 1;
        else
                o->pl = pl;
-       free(sparse);
 }
 
 
--

gitgitgadget

From: Derrick Stolee <[email protected]>

As we modify the sparse-checkout definition, we perform index operations on a pattern_list that only exists in-memory. This allows easy backing out in case the index update fails.

However, if the index write itself cares about the sparse-checkout pattern set, we need access to that in-memory copy. Place a pointer to a 'struct pattern_list' in the index so we can access this on-demand.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c | 17 ++++++++++-------
 cache.h                   |  2 ++
 2 files changed, 12 insertions(+), 7 deletions(-)

builtin/sparse-checkout.c +2 -0

index 2306a9ad98e..e00b82af727 100644
@@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)
        if (is_index_unborn(r->index))
                return UPDATE_SPARSITY_SUCCESS;
 
+       r->index->sparse_checkout_patterns = pl;
+
        memset(&o, 0, sizeof(o));
        o.verbose_update = isatty(2);
        o.update = 1;

builtin/sparse-checkout.c +1 -2

 }
 

@@ -517,19 +520,18 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
 {
        int result;
        int changed_config = 0;
-       struct pattern_list pl;
-       memset(&pl, 0, sizeof(pl));
+       struct pattern_list *pl = xcalloc(1, sizeof(*pl));
 
        switch (m) {
        case ADD:
                if (core_sparse_checkout_cone)

builtin/sparse-checkout.c +3 -2

                break;
        }
 

@@ -539,12 +541,13 @@ static int modify_pattern_list(int argc, const char **argv, enum modify_type m)
                changed_config = 1;
        }
 
-       result = write_patterns_and_update(&pl);
+       result = write_patterns_and_update(pl);
 
        if (result && changed_config)
                set_config(MODE_NO_PATTERNS);
 
-       clear_pattern_list(&pl);
+       clear_pattern_list(pl);
+       free(pl);
        return result;
 }
 

cache.h +2 -1

index f9c7a603841..bf4ec3de4b0 100644
@@ -305,6 +305,7 @@ static inline unsigned int canon_mode(unsigned int mode)
 struct split_index;
 struct untracked_cache;
 struct progress;
+struct pattern_list;
 
 struct index_state {
        struct cache_entry **cache;

@@ -329,6 +330,7 @@ struct index_state {
        struct mem_pool *ce_mem_pool;
        struct progress *progress;
        struct repository *repo;
+       struct pattern_list *sparse_checkout_patterns;
 };
 
 /* Name hashing */
--

gitgitgadget

Quoted 31 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

As we modify the sparse-checkout definition, we perform index operations on a pattern_list that only exists in-memory. This allows easy backing out in case the index update fails.

However, if the index write itself cares about the sparse-checkout pattern set, we need access to that in-memory copy. Place a pointer to a 'struct pattern_list' in the index so we can access this on-demand.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/sparse-checkout.c | 17 ++++++++++-------
 cache.h                   |  2 ++
 2 files changed, 12 insertions(+), 7 deletions(-)

builtin/sparse-checkout.c +2 -0

index 2306a9ad98e..e00b82af727 100644
@@ -110,6 +110,8 @@ static int update_working_directory(struct pattern_list *pl)
        if (is_index_unborn(r->index))
                return UPDATE_SPARSITY_SUCCESS;
 
+       r->index->sparse_checkout_patterns = pl;
+
        memset(&o, 0, sizeof(o));
        o.verbose_update = isatty(2);
        o.update = 1;

builtin/sparse-checkout.c static int update_working_directory(struct pattern_list *pl) · line 144

Quoted 5 lines

builtin/sparse-checkout.c +1 -0

@@ -138,6 +140,7 @@ static int update_working_directory(struct pattern_list *pl)
        else
                rollback_lock_file(&lock_file);
 
+       r->index->sparse_checkout_patterns = NULL;
        return result;

The setting back to NULL made me curious; we don't want this information to remain available later? Is it only going to be used for the updating of the working directory?

I dug a bit into the callers, and didn't find the answer to my question...but I did notice that modify_pattern_list() will correctly free the patterns after write_patterns_and_update() via calling clear_pattern_list(&pl), but sparse_checkout_init() appears to leak the patterns it allocates. That's a separate issue from this patch, but do you want to fix that up while working in this area (so I avoid stepping on your toes with all your other patches)?

From: Derrick Stolee <[email protected]>

Most test cases can verify Git's behavior using input/output expectations or changes to the .git directory. However, sometimes we want to check that Git did or did not run a certain section of code. This is particularly important for performance-only features that we want to ensure have been enabled in certain cases.

Add a new 'test_region' function that checks if a trace2 region was entered and left in a given trace2 event log.

There is one existing test (t0500-progress-display.sh) that performs this check already, so use the helper function instead. More uses will be added in a later change.

t6423-merge-rename-directories.sh also greps for region_enter lines, but it verifies the number of such lines, which is not the same as an existence check.

Signed-off-by Derrick Stolee <[email protected]>

---
 t/t0500-progress-display.sh |  3 +--
 t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

t/t0500-progress-display.sh +0 -0

index 1ed1df351cb..c461b89dfaf 100755

t/t0500-progress-display.sh +0 -0

        grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
        grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
 '

t/test-lib-functions.sh +40 -1

index 999982fe4a9..c878db93013 100644
@@ -1655,3 +1655,43 @@ test_subcommand () {
                grep "\[$expr\]"
        fi
 }
+
+# Check that the given command was invoked as part of the
+# trace2-format trace on stdin.
+#
+#      test_region [!] <category> <label> git <command> <args>...
+#
+# For example, to look for trace2_region_enter("index", "do_read_index", repo)
+# in an invocation of "git checkout HEAD~1", run
+#
+#      GIT_TRACE2_EVENT="$(pwd)/trace.txt" GIT_TRACE2_EVENT_NESTING=10 \
+#              git checkout HEAD~1 &&
+#      test_region index do_read_index <trace.txt
+#
+# If the first parameter passed is !, this instead checks that
+# the given region was not entered.
+#
+test_region () {
+       local expect_exit=0
+       if test "$1" = "!"
+       then
+               expect_exit=1
+               shift
+       fi
+
+       grep -e "region_enter" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
+       exitcode=$?
+
+       if test $exitcode != $expect_exit
+       then
+               return 1
+       fi
+
+       grep -e "region_leave" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
+       exitcode=$?
+
+       if test $exitcode != $expect_exit
+       then
+               return 1
+       fi
+}
--

gitgitgadget

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget <[email protected]> wrote:

t/t0500-progress-display.sh test_expect_success 'progress generates traces' ' · line 306

Quoted 4 lines

        # t0212/parse_events.perl intentionally omits regions and data.

t/t0500-progress-display.sh +1 -2

-       grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
-       grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
+       test_region category progress trace.event &&

Sidenote: Hmm...about 40% of my region labels in merge-ort.c and 90% in diffcore-rename.c have spaces in them. This function could still be used, but I'm curious if I should change the labels (but then again, they are testing logical regions rather than individual functions, and the spaces instead of underscores kind of convey that...)

From: Derrick Stolee <[email protected]>

These also document some behaviors that differ from a full checkout, and possibly in a way that is not intended.

Signed-off-by Derrick Stolee <[email protected]>

---
 t/t1092-sparse-checkout-compatibility.sh | 323 +++++++++++++++++++++++
 1 file changed, 323 insertions(+)
 create mode 100755 t/t1092-sparse-checkout-compatibility.sh

t/t1092-sparse-checkout-compatibility.sh new file +142 -0

new file mode 100755
index 00000000000..46f9dc2cdf3
@@ -0,0 +1,323 @@
+#!/bin/sh
+
+test_description='compare full workdir to sparse workdir'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+       git init initial-repo &&
+       (
+               cd initial-repo &&
+               echo a >a &&
+               echo "after deep" >e &&
+               echo "after folder1" >g &&
+               mkdir folder1 folder2 deep x &&
+               mkdir deep/deeper1 deep/deeper2 &&
+               mkdir deep/deeper1/deepest &&
+               echo "after deeper1" >deep/e &&
+               echo "after deepest" >deep/deeper1/e &&
+               cp a folder1 &&
+               cp a folder2 &&
+               cp a deep &&
+               cp a deep/deeper1 &&
+               cp a deep/deeper2 &&
+               cp a deep/deeper1/deepest &&
+               git add . &&
+               git commit -m "initial commit" &&
+               git checkout -b base &&
+               for dir in folder1 folder2 deep
+               do
+                       git checkout -b update-$dir &&
+                       echo "updated $dir" >$dir/a &&
+                       git commit -a -m "update $dir" || return 1
+               done &&
+
+               git checkout -b rename-base base &&
+               echo >folder1/larger-content <<-\EOF &&
+               matching
+               lines
+               help
+               inexact
+               renames
+               EOF
+               cp folder1/larger-content folder2/ &&
+               cp folder1/larger-content deep/deeper1/ &&
+               git add . &&
+               git commit -m "add interesting rename content" &&
+
+               git checkout -b rename-out-to-out rename-base &&
+               mv folder1/a folder2/b &&
+               mv folder1/larger-content folder2/edited-content &&
+               echo >>folder2/edited-content &&
+               git add . &&
+               git commit -m "rename folder1/... to folder2/..." &&
+
+               git checkout -b rename-out-to-in rename-base &&
+               mv folder1/a deep/deeper1/b &&
+               mv folder1/larger-content deep/deeper1/edited-content &&
+               echo >>deep/deeper1/edited-content &&
+               git add . &&
+               git commit -m "rename folder1/... to deep/deeper1/..." &&
+
+               git checkout -b rename-in-to-out rename-base &&
+               mv deep/deeper1/a folder1/b &&
+               mv deep/deeper1/larger-content folder1/edited-content &&
+               echo >>folder1/edited-content &&
+               git add . &&
+               git commit -m "rename deep/deeper1/... to folder1/..." &&
+
+               git checkout -b deepest base &&
+               echo "updated deepest" >deep/deeper1/deepest/a &&
+               git commit -a -m "update deepest" &&
+
+               git checkout -f base &&
+               git reset --hard
+       )
+'
+
+init_repos () {
+       rm -rf full-checkout sparse-checkout sparse-index &&
+
+       # create repos in initial state
+       cp -r initial-repo full-checkout &&
+       git -C full-checkout reset --hard &&
+
+       cp -r initial-repo sparse-checkout &&
+       git -C sparse-checkout reset --hard &&
+       git -C sparse-checkout sparse-checkout init --cone &&
+
+       # initialize sparse-checkout definitions
+       git -C sparse-checkout sparse-checkout set deep
+}
+
+run_on_sparse () {
+       (
+               cd sparse-checkout &&
+               $* >../sparse-checkout-out 2>../sparse-checkout-err
+       )
+}
+
+run_on_all () {
+       (
+               cd full-checkout &&
+               $* >../full-checkout-out 2>../full-checkout-err
+       ) &&
+       run_on_sparse $*
+}
+
+test_all_match () {
+       run_on_all $* &&
+       test_cmp full-checkout-out sparse-checkout-out &&
+       test_cmp full-checkout-err sparse-checkout-err
+}
+
+test_expect_success 'status with options' '
+       init_repos &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git status --porcelain=v2 -z -u &&
+       test_all_match git status --porcelain=v2 -uno &&
+       run_on_all "touch README.md" &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git status --porcelain=v2 -z -u &&
+       test_all_match git status --porcelain=v2 -uno &&
+       test_all_match git add README.md &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git status --porcelain=v2 -z -u &&
+       test_all_match git status --porcelain=v2 -uno
+'
+
+test_expect_success 'add, commit, checkout' '
+       init_repos &&
+
+       write_script edit-contents <<-\EOF &&
+       echo text >>README.md
+       EOF
+       run_on_all "../edit-contents" &&
+
+       test_all_match git add README.md &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git commit -m "Add README.md" &&
+
+       test_all_match git checkout HEAD~1 &&
+       test_all_match git checkout - &&

t/t1092-sparse-checkout-compatibility.sh new file +14 -0

+
+test_expect_success 'add, commit, checkout' '
+       init_repos &&
+
+       write_script edit-contents <<-\EOF &&
+       echo text >>$1
+       EOF
+       run_on_all "../edit-contents README.md" &&
+
+       test_all_match git add README.md &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git commit -m "Add README.md" &&
+
+       test_all_match git checkout HEAD~1 &&

t/t1092-sparse-checkout-compatibility.sh new file +20 -0

+
+test_expect_success 'checkout and reset --hard' '
+       init_repos &&
+
+       test_all_match git checkout update-folder1 &&
+       test_all_match git status --porcelain=v2 &&
+
+       test_all_match git checkout update-deep &&
+       test_all_match git status --porcelain=v2 &&
+
+       test_all_match git checkout -b reset-test &&
+       test_all_match git reset --hard deepest &&
+       test_all_match git reset --hard update-folder1 &&
+       test_all_match git reset --hard update-folder2
+'
+
+test_expect_success 'diff --staged' '
+       init_repos &&
+
+       write_script edit-contents <<-\EOF &&

t/t1092-sparse-checkout-compatibility.sh new file +15 -0

+       done
+'
+
+test_expect_success 'log with pathspec outside sparse definition' '
+       init_repos &&
+
+       test_all_match git log -- a &&
+       test_all_match git log -- folder1/a &&
+       test_all_match git log -- folder2/a &&
+       test_all_match git log -- deep/a &&
+       test_all_match git log -- deep/deeper1/a &&
+       test_all_match git log -- deep/deeper1/deepest/a &&
+
+       test_all_match git checkout update-folder1 &&
+       test_all_match git log -- folder1/a

t/t1092-sparse-checkout-compatibility.sh new file +47 -0

+test_expect_failure 'checkout and reset (mixed)' '
+       init_repos &&
+
+       test_all_match git checkout -b reset-test update-deep &&
+       test_all_match git reset deepest &&
+       test_all_match git reset update-folder1 &&
+       test_all_match git reset update-folder2
+'
+
+test_expect_success 'merge' '
+       init_repos &&
+
+       test_all_match git checkout -b merge update-deep &&
+       test_all_match git merge -m "folder1" update-folder1 &&
+       test_all_match git rev-parse HEAD^{tree} &&
+       test_all_match git merge -m "folder2" update-folder2 &&
+       test_all_match git rev-parse HEAD^{tree}
+'
+
+test_expect_success 'merge with outside renames' '
+       init_repos &&
+
+       for type in out-to-out out-to-in in-to-out
+       do
+               test_all_match git reset --hard &&
+               test_all_match git checkout -f -b merge-$type update-deep &&
+               test_all_match git merge -m "$type" rename-$type &&
+               test_all_match git rev-parse HEAD^{tree} || return 1
+       done
+'
+
+test_expect_success 'clean' '
+       init_repos &&
+
+       echo bogus >>.gitignore &&
+       run_on_all cp ../.gitignore . &&
+       test_all_match git add .gitignore &&
+       test_all_match git commit -m ignore-bogus-files &&
+
+       run_on_sparse mkdir folder1 &&
+       run_on_all touch folder1/bogus &&
+
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git clean -f &&
+       test_all_match git status --porcelain=v2 &&
+
+       test_all_match git clean -xf &&

Quoted 3 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Quoted 2 lines

These also document some behaviors that differ from a full checkout, and possibly in a way that is not intended.

I'm in favor. I should turn some of my noted weird behaviors from [1] into testcases as well.

[1] https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/

Quoted 18 lines

(quoted diff) +18 -0

+
+test_expect_success 'add, commit, checkout' '
+       init_repos &&
+
+       write_script edit-contents <<-\EOF &&
+       echo text >>$1
+       EOF
+       run_on_all "../edit-contents README.md" &&
+
+       test_all_match git add README.md &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git commit -m "Add README.md" &&
+
+       test_all_match git checkout HEAD~1 &&
+       test_all_match git checkout - &&
+
+       run_on_all "../edit-contents README.md" &&
+

Review

Quoted 6 lines

(quoted diff) +6 -0

+       test_all_match git add -A &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git commit -m "Extend README.md" &&
+
+       test_all_match git checkout HEAD~1 &&
+       test_all_match git checkout - &&

Up to here, this test is identical to the previous one. Why repeat it?

Review

Quoted 12 lines

(quoted diff) +12 -0

+
+       run_on_all "../edit-contents deep/newfile" &&
+
+       test_all_match git status --porcelain=v2 -uno &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git add . &&
+       test_all_match git status --porcelain=v2 &&
+       test_all_match git commit -m "add deep/newfile" &&
+
+       test_all_match git checkout HEAD~1 &&
+       test_all_match git checkout -
+'

Think out loud...so you are only adding files that were not previously tracked and that would have been part of the sparse cone. You aren't trying to add files that would be outside the sparse cone, or manually creating files missing from the working tree due to sparseness and then attempting to add them. (Which is fine, we have to start somewhere with our testing. Also, I think my testcases didn't look at the case you did, and only covered one of these other two cases.)

Quoted 20 lines

(quoted diff) +20 -0

+
+test_expect_success 'checkout and reset --hard' '
+       init_repos &&
+
+       test_all_match git checkout update-folder1 &&
+       test_all_match git status --porcelain=v2 &&
+
+       test_all_match git checkout update-deep &&
+       test_all_match git status --porcelain=v2 &&
+
+       test_all_match git checkout -b reset-test &&
+       test_all_match git reset --hard deepest &&
+       test_all_match git reset --hard update-folder1 &&
+       test_all_match git reset --hard update-folder2
+'
+
+test_expect_success 'diff --staged' '
+       init_repos &&
+
+       write_script edit-contents <<-\EOF &&

Review

Quoted 10 lines

(quoted diff) +10 -0

+       echo text >>README.md
+       EOF
+       run_on_all "../edit-contents" &&
+
+       test_all_match git diff &&
+       test_all_match git diff --staged &&
+       test_all_match git add README.md &&
+       test_all_match git diff &&
+       test_all_match git diff --staged
+'

Also a case where you're operating on a file that matches the sparsity patterns (in cone mode, everything in the root directory is included).

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_expect_success 'diff with renames' '
+       init_repos &&
+
+       for branch in rename-out-to-out rename-out-to-in rename-in-to-out
+       do
+               test_all_match git checkout rename-base &&
+               test_all_match git checkout $branch -- .&&
+               test_all_match git diff --staged &&
+               test_all_match git diff --staged --find-renames || return 1

Aren't these last two lines the same? (diff.renames defaults to true ever since commit 5404c116aa, "diff: activate diff.renames by default", 2016-02-25) Are they only different because you have a tweaked config that turns off renames by default?

Perhaps the first diff line should have a --no-renames flag.

Quoted 15 lines

(quoted diff) +15 -0

+       done
+'
+
+test_expect_success 'log with pathspec outside sparse definition' '
+       init_repos &&
+
+       test_all_match git log -- a &&
+       test_all_match git log -- folder1/a &&
+       test_all_match git log -- folder2/a &&
+       test_all_match git log -- deep/a &&
+       test_all_match git log -- deep/deeper1/a &&
+       test_all_match git log -- deep/deeper1/deepest/a &&
+
+       test_all_match git checkout update-folder1 &&
+       test_all_match git log -- folder1/a

Review

Quoted 10 lines

(quoted diff) +10 -0

+'
+
+test_expect_success 'blame with pathspec inside sparse definition' '
+       init_repos &&
+
+       test_all_match git blame a &&
+       test_all_match git blame deep/a &&
+       test_all_match git blame deep/deeper1/a &&
+       test_all_match git blame deep/deeper1/deepest/a
+'

Good check.

On a side note going back to a piece of the other thread I didn't get a response to, I'm still curious whether
git blame -C -C $PATH_INSIDE_SPARSE_DEFINITION
should (optionally?) behave differently in a sparse checkout. In particular, should it limit its copy detection to other paths also in the sparse checkout, or should it always search all other files within the repository for copied lines? Searching just within the sparse checkout seems like it could be a really nice performance optimization.

Review

Quoted 3 lines

(quoted diff) +3 -0

+
+# TODO: blame currently does not support blaming files outside of the
+# sparse definition. It complains that the file doesn't exist locally.

Nice catch. Yeah, blame tries to check the local working copy for changes, and shows those lines with a changed in commit 0000000000. We should add a check that says that if the file is SKIP_WORKTREE, then we treat it the same as git blame $PATH HEAD.

Review

Quoted 11 lines

(quoted diff) +11 -0

+test_expect_failure 'blame with pathspec outside sparse definition' '
+       init_repos &&
+
+       test_all_match git blame folder1/a &&
+       test_all_match git blame folder2/a &&
+       test_all_match git blame deep/deeper2/a &&
+       test_all_match git blame deep/deeper2/deepest/a
+'
+
+# TODO: reset currently does not behave as expected when in a
+# sparse-checkout.

I'm going to go to test this out to see what it does. It's the first testcase you listed that I didn't know how it worked and couldn't figure it out from your comments. However it turns out, definitely a good test to have.

Review

Quoted 8 lines

(quoted diff) +8 -0

+test_expect_failure 'checkout and reset (mixed)' '
+       init_repos &&
+
+       test_all_match git checkout -b reset-test update-deep &&
+       test_all_match git reset deepest &&
+       test_all_match git reset update-folder1 &&
+       test_all_match git reset update-folder2
+'

From: Derrick Stolee <[email protected]>

Make the method safer by allocating a cache_tree member for the given index_state if it is not already present. This is preferrable to a BUG() statement or returning with an error because future callers will want to populate an empty cache-tree using this method.

Also drop local variables that are used exactly once and can be found directly from the 'istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache-tree.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

cache-tree.c +9 -5

index 3f1a8d4f1b7..c1e49901c17 100644
@@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,
 
 int cache_tree_update(struct index_state *istate, int flags)
 {
-	struct cache_tree *it = istate->cache_tree;
-	struct cache_entry **cache = istate->cache;
-	int entries = istate->cache_nr;
-	int skip, i = verify_cache(cache, entries, flags);
+	int skip, i;
+
+	i = verify_cache(istate->cache, istate->cache_nr, flags);
 
 	if (i)
 		return i;
+
+	if (!istate->cache_tree)
+		istate->cache_tree = cache_tree();
+
 	trace_performance_enter();
 	trace2_region_enter("cache_tree", "update", the_repository);
-	i = update_one(it, cache, entries, "", 0, &skip, flags);
+	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
+		       "", 0, &skip, flags);
 	trace2_region_leave("cache_tree", "update", the_repository);
 	trace_performance_leave("cache_tree_update");
 	if (i < 0)

From: Derrick Stolee <[email protected]>

The index has an fsmonitor_dirty bitmap that records which index entries are "dirty" based on the response from the FSMonitor. If this bitmap ever grows larger than the index, then there was an error in how it was constructed, and it was probably a developer's bug.

There are several BUG() statements that are very similar, so replace these uses with a simpler assert_index_minimum(). Since there is one caller that uses a custom 'pos' value instead of the bit_size member, we cannot simplify it too much. However, the error string is identical in each, so this simplifies things.

The end result is that the code is simpler to read while also preserving these assertions for developers in the FSMonitor space.

Signed-off-by Derrick Stolee <[email protected]>

---
 fsmonitor.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

fsmonitor.c +13 -14

index ca031c3abb8..52a50a9545a 100644
@@ -13,14 +13,19 @@
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 
+static void assert_index_minimum(struct index_state *istate, size_t pos)
+{
+	if (pos > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
+		    (uintmax_t)pos, istate->cache_nr);
+}
+
 static void fsmonitor_ewah_callback(size_t pos, void *is)
 {
 	struct index_state *istate = (struct index_state *)is;
 	struct cache_entry *ce;
 
-	if (pos >= istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
-		    (uintmax_t)pos, istate->cache_nr);
+	assert_index_minimum(istate, pos);
 
 	ce = istate->cache[pos];
 	ce->ce_flags &= ~CE_FSMONITOR_VALID;

@@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	}
 	istate->fsmonitor_dirty = fsmonitor_dirty;
 
-	if (!istate->split_index &&
-	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index)
+		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
 	return 0;

@@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	if (!istate->split_index &&
-	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index)
+		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));

@@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)
 			}
 
 			/* Mark all previously saved entries as dirty */
-			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+			assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
 			refresh_fsmonitor(istate);

From: Derrick Stolee <[email protected]>

It will be helpful to add behavior to index operations that might trigger an object lookup. Since each index belongs to a specific repository, add a 'repo' pointer to struct index_state that allows access to this repository.

This will prevent future changes from needing to pass an additional 'struct repository *repo' parameter and instead rely only on the 'struct index_state *istate' parameter.

Signed-off-by Derrick Stolee <[email protected]>

---

return read_index_from(repo->index, repo->index_file, repo->gitdir);
}

t/test-lib-functions.sh +7 -0

+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+}

t/test-lib-functions.sh test_subcommand () { · line 1682

Quoted 1 line · Derrick Stolee via GitGitGadget

t/test-lib-functions.sh +1 -0

+	grep -e "\"region_enter\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

... this makes sure there is enter/category on a line (and leave/category on a line with another check). Makes sense.

But...

test_region '!' '\(unmatching capture)' 'two' 'three'

would try to use an invalid regexp and cause grep to exit with 2, which would mean ...

t/test-lib-functions.sh test_subcommand () { · line 1685

Quoted 3 lines

t/test-lib-functions.sh +3 -0

+	exitcode=$?
+
+	if test $exitcode != $expect_exit

... this will not trigger and we return "success" (i.e. "failed as expected")?

Clarification. The point is *NOT* that the grep pattern is not robust against funnies in $1 and $2---after all, these strings are under our control. The point is what should happen when "grep" exits with an error when asked to ensure that there is no region detected.

t/test-lib-functions.sh test_subcommand () { · line 1690

Quoted 1 line

t/test-lib-functions.sh +1 -0

+	grep -e "\"region_leave\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

The same comment on "what about an error from grep" applies to this one.

It might be easier to read to avoid having to say too many backslash-quoted double quotes:

	grep -e	'"region_leave".*"category":"'"$1"'","label":"'"$2"\" "$3"

This comment applies to the earlier "grep", too.

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	if test $exitcode != $expect_exit

... this will not trigger and we return "success" (i.e. "failed as expected")?

Am I misunderstanding something here? If exitcode is 2, then this will always trigger and return 1, signaling a failure.

This is based on ds/cache-tree-basics.

Here are a few more cleanups that are vaguely related to the index. I discovered these while preparing my sparse-index RFC that I intend to send early next week.

The biggest patch is the final one, which creates a test script for comparing sparse-checkouts to full checkouts. There are some commands that do not behave similarly. This script will be the backbone of my testing strategy for the sparse-index by adding a new mode to compare sparse-checkouts with the two index types (full and sparse).

UPDATES IN V2
=============

* Fixed duplicated test in t1092.

* Changed the implementation of 'test_region' after I discovered the negation doesn't work correctly. (I updated the test to use what was in t0500-progress-display.sh at the last minute before v1, but that implementation was wrong.) The use of it in t0500-progress-display.sh was incorrect, as well.

* Updated commit messages to be more informative and have fewer typos.

* I dropped the patch that placed the sparse-checkout patterns in struct index_state. I'll re-introduce that in time for the actual use of the member.

You've addressed all my feedback from v1, but it looks like you missed the pos + 1 changes highlighted by Chris in his review of patch 3. Oversight?

From: Derrick Stolee <[email protected]>

The verify_cache() method takes an array of cache entries and a count, but these are always provided directly from a struct index_state. Use a pointer to the full structure instead.

There is a subtle point when istate->cache_nr is zero that subtracting one will underflow. This triggers a failure in t0000-basic.sh, among others. Use "i + 1 < istate->cache_nr" to avoid these strange comparisons. Convert i to be unsigned as well, which also removes the potential signed overflow in the unlikely case that cache_nr is over 2.1 billion entries. The 'funny' variable has a maximum value of 11, so

making it unsigned does not change anything of importance.

Signed-off-by Derrick Stolee <[email protected]>

---
 cache-tree.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

cache-tree.c +8 -10

index 60b6aefbf51..acac6d58c37 100644
@@ -151,16 +151,15 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)
                istate->cache_changed |= CACHE_TREE_CHANGED;
 }
 
-static int verify_cache(struct cache_entry **cache,
-                       int entries, int flags)
+static int verify_cache(struct index_state *istate, int flags)
 {
-       int i, funny;
+       unsigned i, funny;
        int silent = flags & WRITE_TREE_SILENT;
 
        /* Verify that the tree is merged */
        funny = 0;
-       for (i = 0; i < entries; i++) {
-               const struct cache_entry *ce = cache[i];
+       for (i = 0; i < istate->cache_nr; i++) {
+               const struct cache_entry *ce = istate->cache[i];
                if (ce_stage(ce)) {
                        if (silent)
                                return -1;

@@ -180,13 +179,13 @@ static int verify_cache(struct cache_entry **cache,
         * stage 0 entries.
         */
        funny = 0;
-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];
                const char *this_name = this_ce->name;
                const char *next_name = next_ce->name;
                int this_len = ce_namelen(this_ce);

@@ -438,7 +437,7 @@ int cache_tree_update(struct index_state *istate, int flags)
 {
        int skip, i;
 
-       i = verify_cache(istate->cache, istate->cache_nr, flags);
+       i = verify_cache(istate, flags);
 
        if (i)
                return i;
--

gitgitgadget

cache-tree.c static int verify_cache(struct cache_entry **cache, · line 191

Quoted 13 lines · Derrick Stolee via GitGitGadget

cache-tree.c +3 -3

-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];
                const char *this_name = this_ce->name;
                const char *next_name = next_ce->name;
                int this_len = ce_namelen(this_ce);

Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit in the commit message; made it easier to read through quickly. I'm curious if it deserves a comment in the code too, since it does feel slightly unusual.

Quoted 15 lines · Elijah Newren

cache-tree.c static int verify_cache(struct cache_entry **cache, · line 191

Quoted 13 lines · Derrick Stolee via GitGitGadget

cache-tree.c +3 -3

-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];
                const char *this_name = this_ce->name;
                const char *next_name = next_ce->name;
                int this_len = ce_namelen(this_ce);

Quoted 4 lines

Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit in the commit message; made it easier to read through quickly. I'm curious if it deserves a comment in the code too, since it does feel slightly unusual.

I would argue that "i + 1 < N" is a more natural way to write this, because we use "i + 1" as an index, so we want to ensure the index we are about to use is within range. "i < N - 1" is the backwards way to write that statement.

Quoted 15 lines · Elijah Newren

cache-tree.c static int verify_cache(struct cache_entry **cache, · line 191

Quoted 13 lines · Derrick Stolee via GitGitGadget

cache-tree.c +3 -3

-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];
                const char *this_name = this_ce->name;
                const char *next_name = next_ce->name;
                int this_len = ce_namelen(this_ce);

Quoted 4 lines

Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit in the commit message; made it easier to read through quickly. I'm curious if it deserves a comment in the code too, since it does feel slightly unusual.

I would argue that "i + 1 < N" is a more natural way to write this, because we use "i + 1" as an index, so we want to ensure the index we are about to use is within range. "i < N - 1" is the backwards way to write that statement.

cache-tree.c static int verify_cache(struct cache_entry **cache, · line 188

Quoted 10 lines

cache-tree.c +3 -3

-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];

Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit in the commit message; made it easier to read through quickly. I'm curious if it deserves a comment in the code too, since it does feel slightly unusual.

Quoted 11 lines · Elijah Newren

cache-tree.c static int verify_cache(struct cache_entry **cache, · line 188

Quoted 10 lines

cache-tree.c +3 -3

-       for (i = 0; i < entries - 1; i++) {
+       for (i = 0; i + 1 < istate->cache_nr; i++) {
                /* path/file always comes after path because of the way
                 * the cache is sorted.  Also path can appear only once,
                 * which means conflicting one would immediately follow.
                 */
-               const struct cache_entry *this_ce = cache[i];
-               const struct cache_entry *next_ce = cache[i + 1];
+               const struct cache_entry *this_ce = istate->cache[i];
+               const struct cache_entry *next_ce = istate->cache[i + 1];

Quoted 4 lines

Makes sense. Thanks for explaining the i + 1 < istate->cache_nr bit in the commit message; made it easier to read through quickly. I'm curious if it deserves a comment in the code too, since it does feel slightly unusual.

I think this is entirely my fault.

It probably reads more natural to start from 1 and interate through to the end of the array, comparing the current one with the previous entry, i.e.

	for (i = 1; i < istate->cache_nr; i++) {
        	prev = cache[i - 1];
		this = cache[i];
                compare(prev, this);