On 1/20/2021 1:03 PM, Elijah Newren wrote: > On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxx> >> --- >> 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
