[7/9] sparse-checkout: hold pattern list in index

Commit Message

Derrick Stolee Jan. 20, 2021, 4:53 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
---
 builtin/sparse-checkout.c | 17 ++++++++++-------
 cache.h                   |  2 ++
 2 files changed, 12 insertions(+), 7 deletions(-)

Comments

Elijah Newren Jan. 20, 2021, 6:03 p.m. UTC | #1

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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.

Derrick Stolee Jan. 20, 2021, 7:22 p.m. UTC | #2

On 1/20/2021 1:03 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Derrick Stolee <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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

Patch

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;
 }
 
@@ -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);
 		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 */