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

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 7/9] sparse-checkout: hold pattern list in index
Date: Wed, 20 Jan 2021 14:22:33 -0500	[thread overview]
Message-ID: <fb1800fb-474a-e20b-5596-6bb3bc22fe99@gmail.com> (raw)
In-Reply-To: <CABPp-BEeDj4v4t8w6giy-yo8ET0mnD3es95aGQJvw7g8_jkjzA@mail.gmail.com>

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