Re: [PATCH v2 03/10] sparse-index: create expand_to_pattern_list()
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- May 20, 2022, 18:01 UTC
- Message-ID
- <3d4cb244-2e55-0337-90f8-a380ed836465@github.com>
- In-Reply-To
- <xmqqsfp5mhcw.fsf@gitster.g>
On 5/19/2022 3:50 PM, Junio C Hamano wrote:
Show 17 quoted lines
> "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.
Show 19 quoted lines
>> - 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