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

On 1/20/2021 1:03 PM, Elijah Newren wrote:

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

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