Skip to content

More index cleanups #839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
derrickstolee wants to merge 9 commits into from
Closed

More index cleanups #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bdc8ecc
cache-tree: clean up cache_tree_update()
derrickstolee Jan 8, 2021
1b8b568
cache-tree: simplify verify_cache() prototype
derrickstolee Jan 23, 2021
314b6b3
cache-tree: extract subtree_pos()
derrickstolee Jan 12, 2021
4e688d2
fsmonitor: de-duplicate BUG()s around dirty bits
derrickstolee Dec 30, 2020
6373997
repository: add repo reference to index_state
derrickstolee Jan 8, 2021
9b545d7
name-hash: use trace2 regions for init
derrickstolee Jan 11, 2021
554cc76
sparse-checkout: load sparse-checkout patterns
derrickstolee Jan 8, 2021
b37181b
test-lib: test_region looks for trace2 regions
derrickstolee Jan 11, 2021
72f9253
t1092: test interesting sparse-checkout scenarios
derrickstolee Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions builtin/checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,9 +821,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
}
}

if (!active_cache_tree)
active_cache_tree = cache_tree();

if (!cache_tree_fully_valid(active_cache_tree))
cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);

Expand Down
5 changes: 0 additions & 5 deletions builtin/sparse-checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ static char const * const builtin_sparse_checkout_usage[] = {
NULL

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  builtin/sparse-checkout.c |  5 -----
>  dir.c                     | 17 +++++++++++++++++
>  dir.h                     |  2 ++
>  unpack-trees.c            |  6 +-----
>  4 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index e3140db2a0a..2306a9ad98e 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -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;
> diff --git a/dir.c b/dir.c
> index d637461da5c..d153a63bbd1 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -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;
> diff --git a/dir.h b/dir.h
> index a3c40dec516..facfae47402 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -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: */
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index af6e9b9c2fd..837b8bb42fb 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -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

Looks straightforward and well motivated to me.

But the cherry on top that really sells this patch is that more lines
of dir.c will blame to someone besides me.  Win-win!

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  builtin/sparse-checkout.c | 17 ++++++++++-------
>  cache.h                   |  2 ++
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 2306a9ad98e..e00b82af727 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -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;
> @@ -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)?

>  }
>
> @@ -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)
> -                       add_patterns_cone_mode(argc, argv, &pl);
> +                       add_patterns_cone_mode(argc, argv, pl);
>                 else
> -                       add_patterns_literal(argc, argv, &pl);
> +                       add_patterns_literal(argc, argv, pl);
>                 break;
>
>         case REPLACE:
> -               add_patterns_from_input(&pl, argc, argv);
> +               add_patterns_from_input(pl, argc, argv);

Slightly annoying that the other functions are (argc, argv, pl) and
this one is (pl, argc, argv).  But again, that's outside the scope of
this patch and might not be worth the churn to fix.

>                 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;
>  }
>
> diff --git a/cache.h b/cache.h
> index f9c7a603841..bf4ec3de4b0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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

Otherwise, this patch looks good to me.

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/20/2021 1:03 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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 <dstolee@microsoft.com>
>> ---
>>  builtin/sparse-checkout.c | 17 ++++++++++-------
>>  cache.h                   |  2 ++
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 2306a9ad98e..e00b82af727 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -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;
>> @@ -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)?

The thing that caught me here is that update_working_directory() uses
an in-memory pattern_list that hasn't been committed to the
sparse-checkout file yet. This means we need to (temporarily) point
to this pattern_list.

Perhaps this patch is premature, since nothing actually _uses_
sparse_checkout_patterns yet. When we do add such a use, it will
initialize a NULL value with the patterns in the sparse-checkout
file. In that case, we definitely want to inject our in-memory
patterns instead.

Thanks,
-Stolee

};

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;
Expand Down
38 changes: 19 additions & 19 deletions cache-tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static int subtree_name_cmp(const char *one, int onelen,
return memcmp(one, two, onelen);

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  cache-tree.c | 6 +++---
>  cache-tree.h | 2 ++
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index c1e49901c17..2b130dd5e19 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -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]);
> diff --git a/cache-tree.h b/cache-tree.h
> index 639bfa5340e..8efeccebfc9 100644
> --- a/cache-tree.h
> +++ b/cache-tree.h
> @@ -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

Simple, straight-forward patch for exposing the function outside the
file scope; looks good.

}

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;
Expand All @@ -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)
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -151,16 +151,15 @@ void cache_tree_invalidate_path(struct index_state *istate, const char *path)
istate->cache_changed |= CACHE_TREE_CHANGED;

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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

AND a minimum value of 0 (which is important for the type change to be valid).

> making it unsigned does not change anything of importance.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  cache-tree.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 60b6aefbf51..acac6d58c37 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -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

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.

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/23/2021 3:24 PM, Elijah Newren wrote:
> On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> -       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.

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.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

>> -       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.

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

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jan 23, 2021 at 1:02 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/23/2021 3:24 PM, Elijah Newren wrote:
> > On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> -       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.
>
> 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.

Oh, right, I think I was reading too quickly and assuming one thing in
my head (about what the code was going to do), and forgetting that
assumption when I got to the actual code.  Sorry about that; I agree
with you, so ignore my previous comment.

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/23/2021 4:10 PM, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
> 
>>> -       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.
> 
> 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);

This would be another natural way to make the loop extremely clear.

It's a bigger diff, changing the names of 'this' and 'next', but
perhaps worthwhile.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> On 1/23/2021 3:24 PM, Elijah Newren wrote:
>> On Sat, Jan 23, 2021 at 11:58 AM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> -       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.
>
> 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.

Our mails have crossed, I guess.  Comparing i+1 and N is also good.

}

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;
Expand All @@ -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);
Expand Down Expand Up @@ -436,16 +435,20 @@ static int update_one(struct cache_tree *it,

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  cache-tree.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 3f1a8d4f1b7..c1e49901c17 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -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);

All mechanical changes so far; these look obviously correct.

>
>         if (i)
>                 return i;
> +
> +       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?)

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

Another mechanical update; looks good.

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

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/20/2021 12:21 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:...
>> +
>> +       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?)

It's something I need in the future when I use the cache_tree_update()
in more places. I think I call it two times, and either I need to
initialize the cache_tree member outside of both, or just make it a
feature of the method that it will re-initialize the cache-tree.

Note: the implementation treats an initialized, but empty cache-tree
as "invalid" so update_one() correctly populates the full tree.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Jan 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

How do the current callers prepare the istate to be passed to this
function?  The implications of this question obviously are:

 - why is it unreasonable for future callers to follow suit?  or

 - now the callers are no longer responsible to give an empty
   cache_tree to istate before calling this function, shouldn't
   existing callers lose code that is no longer needed?

I would imagine that the first is easily answered with "but that
would be more code on the callers' side", but then the second one
becomes even more relevant, no?

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

It actuall is used twice, no?

I do find it an improvement because it makes it clearer that
istate->{cache,cache_nr} comes in pairs.

I wonder if verify_cache() wants to take istate as a parameter,
replacing the first two?  update_one() shifts where it starts
working in the array and reduces the number of entries as it digs
deeper, so it still has to keep taking the (base, nr) pair (iow, its
second and third parameters cannot be replaced with an istate).

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  cache-tree.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 3f1a8d4f1b7..c1e49901c17 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -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)

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, 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)
Expand Down Expand Up @@ -635,9 +638,6 @@ static int write_index_as_tree_internal(struct object_id *oid,
cache_tree_valid = 0;
}

if (!index_state->cache_tree)
index_state->cache_tree = cache_tree();

if (!cache_tree_valid && cache_tree_update(index_state, flags) < 0)
return WRITE_TREE_UNMERGED_INDEX;

Expand Down
2 changes: 2 additions & 0 deletions cache-tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 1 addition & 0 deletions cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ struct index_state {
struct ewah_bitmap *fsmonitor_dirty;

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> It will be helpful to add behavior to index opertations that might

s/opertations/operations/

> 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 <dstolee@microsoft.com>
> ---
>  cache.h      | 1 +
>  repository.c | 4 ++++
>  2 files changed, 5 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 71097657489..f9c7a603841 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -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 */
> diff --git a/repository.c b/repository.c
> index a4174ddb062..67a4c1da2d9 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -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

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.

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/20/2021 12:46 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> It will be helpful to add behavior to index opertations that might
> 
> s/opertations/operations/

Thanks.

>> 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 <dstolee@microsoft.com>
>> ---
>>  cache.h      | 1 +
>>  repository.c | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index 71097657489..f9c7a603841 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -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 */
>> diff --git a/repository.c b/repository.c
>> index a4174ddb062..67a4c1da2d9 100644
>> --- a/repository.c
>> +++ b/repository.c
>> @@ -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
> 
> 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).

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

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

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 11:16 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 1/20/2021 12:46 PM, Elijah Newren wrote:
> > On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> It will be helpful to add behavior to index opertations that might
> >
> > s/opertations/operations/
>
> Thanks.
>
> >> 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 <dstolee@microsoft.com>
> >> ---
> >>  cache.h      | 1 +
> >>  repository.c | 4 ++++
> >>  2 files changed, 5 insertions(+)
> >>
> >> diff --git a/cache.h b/cache.h
> >> index 71097657489..f9c7a603841 100644
> >> --- a/cache.h
> >> +++ b/cache.h
> >> @@ -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 */
> >> diff --git a/repository.c b/repository.c
> >> index a4174ddb062..67a4c1da2d9 100644
> >> --- a/repository.c
> >> +++ b/repository.c
> >> @@ -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
> >
> > 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).
>
> > 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).

I'm not sure what I did was a good design decision; I was kind of
exploring and trying to figure things out.  In retrospect, I think it
was probably a bad idea.  But we have various guard rails in the form
of BUG() calls and such when basic assumptions are violated, and here
it seems that you are now making a new basic assumption that an index
belongs to only one repository.  (Even if all current callers happen
to satisfy that assumption, it's not clear to me that git previously
cared if this condition were satisfied or not).  Hence my question
about safety checks.

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

I'm less concerned with your patch as-is (I think your assumption
seems reasonable and I'm fine with labelling my former unsubmitted
patches as erroneous), and more wondering whether others in the future
will accidentally violate assumptions your patch starts encoding...and
whether we can or should do anything about it.  If there's a simple
place we can add a check for such an error, then it probably makes
sense to add one.  If there isn't...then at least we considered it?

@gitgitgadget gitgitgadget Bot Jan 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---

I think this makes sense, but shouldn't we insist on these
bidirectional links to point at each other?  Otherwise we cannot
simplify the function signatures safely later.  That is ...

> +	/* Complete the double-reference */
> +	if (!repo->index->repo)
> +		repo->index->repo = repo;
> +

	else if (repo->index->repo != repo)
		BUG("the repo->index instance does not belong to the repo???");

... a check like this?

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

struct mem_pool *ce_mem_pool;
struct progress *progress;
struct repository *repo;
};

/* Name hashing */
Expand Down
17 changes: 17 additions & 0 deletions dir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions dir.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: */

Expand Down
27 changes: 13 additions & 14 deletions fsmonitor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,19 @@

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

Indeed, looking through the patch, the end result is simpler to read.
Nice cleanup.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  fsmonitor.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ca031c3abb8..52a50a9545a 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -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
>

@gitgitgadget gitgitgadget Bot Jan 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

Also that single caller with a custom 'pos' used to allow 'pos' to
point one beyond the end of istate->cache[] array, but now it is
forbidden.  If this is a strict bugfix, it probably deserves a
mention here.

> 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 <dstolee@microsoft.com>
> ---
>  fsmonitor.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ca031c3abb8..52a50a9545a 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -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);

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

ce = istate->cache[pos];
ce->ce_flags &= ~CE_FSMONITOR_VALID;
Expand Down Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions name-hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  name-hash.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/name-hash.c b/name-hash.c
> index 5d3c7b12c18..4e03fac9bb1 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -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

Yaay for trace2.  :-)

Total sidenote: Are we ever going to get rid of the trace1 stuff?  I'm
sure this was discussed somewhere, but I don't know where.

#include "cache.h"
#include "thread-utils.h"
#include "trace2.h"

struct dir_entry {
struct hashmap_entry ent;
Expand Down Expand Up @@ -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);

Expand All @@ -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");
}

Expand Down
6 changes: 6 additions & 0 deletions repository.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,12 @@ 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;
else if (repo->index->repo != repo)
BUG("repo's index should point back at itself");

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

Expand Down
3 changes: 0 additions & 3 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,9 +679,6 @@ static int do_recursive_merge(struct repository *r,

static struct object_id *get_cache_tree_oid(struct index_state *istate)
{
if (!istate->cache_tree)
istate->cache_tree = cache_tree();

if (!cache_tree_fully_valid(istate->cache_tree))
if (cache_tree_update(istate, 0)) {
error(_("unable to update cache tree"));
Expand Down
3 changes: 1 addition & 2 deletions t/t0500-progress-display.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '
"Working hard" <in 2>stderr &&

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

Ooh, others do this too?  Sounds like a helpful function to add, but
just checking for entered and left means that...

>
> 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.

...yeah, won't cover the case that I added.  That's fine, since it
appears to be a one-off for now and we don't know of any other cases,
current or planned, that want to do something like that yet.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t0500-progress-display.sh |  3 +--
>  t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
> index 1ed1df351cb..c461b89dfaf 100755
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '
>                 "Working hard" <in 2>stderr &&
>
>         # t0212/parse_events.perl intentionally omits regions and data.
> -       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...)

>         grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
>         grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
>  '
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 999982fe4a9..c878db93013 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -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

This patch looks good to me.

@gitgitgadget gitgitgadget Bot Jan 20, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/20/2021 1:20 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
...
>>         # t0212/parse_events.perl intentionally omits regions and data.
>> -       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...)

You should be able to use

	test_region "category with spaces" "progress with spaces" trace

but if not, then the test_region helper could be improved to match.

I do think that it's better to avoid spaces in these identifiers.

Thanks,
-Stolee

@gitgitgadget gitgitgadget Bot Jan 22, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ... Note that this
> changes the expectations slightly. The old test (incorrectly) used two
> patterns for the 'grep' invocation, but this performs an OR of the
> patterns, not an AND. This means that as long as one region_enter event
> was logged, the test would succeed, even if it was not due to the
> progress category.

>  	# t0212/parse_events.perl intentionally omits regions and data.
> -	grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
> -	grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
> +	test_region progress "Working hard" trace.event &&

So we do want "region_enter" and the "category":"progress" on the
same line in the event file, but as long as "category":"progress"
exists, both will pass, regardless of enter/leave.  And ...

> +test_region () {
> +	local expect_exit=0
> +	if test "$1" = "!"
> +	then
> +		expect_exit=1
> +		shift
> +	fi
> +
> +	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 ...

> +	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.

> +	then
> +		return 1
> +	fi
> +
> +	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.

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

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/22/2021 2:42 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> +	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 ...
> 
>> +	exitcode=$?
>> +
>> +	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. That would
propagate to the parent test and cause the test to fail. That seems
like the correct intention, but I'm not 100% confident about that.

> 	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.

I'll be more robust to these in the next version. We'll expect
exit code equal to zero or _not_ equal to zero, depending on the
presence of '!'. This has the downside of returning success for
bad input strings when '!' is specified.

Basically, the approach I'm taking for v3 is here:

	if [test $expect_exit = 1] && [test $exitcode = 0]
	then
		return 1
	elif [test $expect_exit = 0] && [test $exitcode != 0]
	then
		return 1
	fi

>> +	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.

Thanks. This does look a bit cleaner.

-Stolee

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

>>> +	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.

Are, please disregard.  Yes, the above does the right thing already.

@gitgitgadget gitgitgadget Bot Jan 23, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/23/2021 2:58 PM, Derrick Stolee via GitGitGadget wrote:
...
> +	if test $exitcode != $expect_exit = 1]
...
> +	if test $exitcode != $expect_exit = 1]
As Elijah pointed out, these lines are bogus. I'm not sure how
they passed the tests without failure, but here is a replacement
for this patch:

--- >8 ---


From ff15d509b89edd4830d85d53cea3079a6b0c1c08 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <dstolee@microsoft.com>
Date: Mon, 11 Jan 2021 08:53:09 -0500
Subject: [PATCH 8/9] test-lib: test_region looks for trace2 regions

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. Note that this
changes the expectations slightly. The old test (incorrectly) used two
patterns for the 'grep' invocation, but this performs an OR of the
patterns, not an AND. This means that as long as one region_enter event
was logged, the test would succeed, even if it was not due to the
progress category.

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 <dstolee@microsoft.com>
---
 t/t0500-progress-display.sh |  3 +--
 t/test-lib-functions.sh     | 42 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 1ed1df351c..84cce345e7 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '
 		"Working hard" <in 2>stderr &&
 
 	# t0212/parse_events.perl intentionally omits regions and data.
-	grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
-	grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
+	test_region progress "Working hard" trace.event &&
 	grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
 	grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
 '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999982fe4a..9fc4cf8476 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1655,3 +1655,45 @@ 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".*"category":"'"$1"'","label":"'"$2"\" "$3"
+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+
+	grep -e	'"region_leave".*"category":"'"$1"'","label":"'"$2"\" "$3"
+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+
+	return 0
+}
-- 
2.30.0


# t0212/parse_events.perl intentionally omits regions and data.
grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
test_region progress "Working hard" trace.event &&
grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
'
Expand Down
Loading