Re: [PATCH v2 03/10] sparse-index: create expand_to_pattern_list()

[Derrick Stolee <derrickstolee@github.com>]

On 5/19/2022 3:50 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> list is NULL as that does the same as ensure_full_index(). In fact,
>> ensure_full_index() is converted to a shim over
>> expand_to_pattern_list().
> 
> Sounds like a natural evolution of the API that used to be
> all-or-none to expand-only-those-that-match.
> 
> The old one had a sensible name to tell us that it is about the
> in-core index (and "full index" implied it was about sparse-index
> feature because what state other than "full" the index can be---some
> are shrunk into tree entries, which by definition is the
> sparse-index feature).  Contrasted to that, the name of the new one
> is horrible.  It does not even have index anywhere in the name.
> 
> I wonder expand_index() would work?

Makes sense. Good suggestion.
 
>> -	trace2_region_enter("index", "ensure_full_index", istate->repo);
>> +	/*
>> +	 * A NULL pattern set indicates we are expanding a full index, so
>> +	 * we use a special region name that indicates the full expansion.
>> +	 * This is used by test cases, but also helps to differentiate the
>> +	 * two cases.
>> +	 */
> 
> Except that we lost the distinction for non-cone mode, which I am
> not sure matters, but I suspect we do not have to, if we do not want
> to.  Nobody used "pl" up to this point, so resetting it to NULL can
> be done much later.  In later phases of this series, we add another
> case where we can lose pl even if we are not using cone mode, so
> this distinction may start to matter later.  I dunno.
> 
> I'd invent a separate "const char *tr2_region_label" variable and
> set it at the beginning, regardless of where we clobber pl and why,
> and use that label variable for trace2 calls, if I were doing this
> patch.  That feels much simpler and cleaner.

Good idea.

Thanks,
-Stolee