Re: [PATCH v3 00/32] object-store: lookup_commit

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 29, 2018, 19:37 UTC
Message-ID: <xmqqvaa1tme8.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAGZ79kb=W5_3FB6qSinPpcPzRgAe-qkuOCkXTjs6RLqUTjoboA@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 15 quoted lines

> On Fri, Jun 29, 2018 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > One technique these (not just this) recent efforts seem to be
>> > forgetting is to introduce "new" names that take a_repo and then
>> > make the existing one a thin wrapper that calls the new one with
>> > &the_repo as the argument.
>
> So you'd rather want to see it less invasive done similar to
> NO_THE_INDEX_COMPATIBILITY_MACROS ? Someone (jrnieder?)
> called that a failed experiment, as now we need to carry that baggage
> for quite some time and never cleaned up the started migration;
> only recently Duy started to kill off the_index, which would finish
> that migration?

I do not think it was a failed experiment at all. In fact, most of our code is still happy with the single in-core instance of the_index, and I think it is a mistake to trying to use the variant that take an istate instance as parameter just for the sake of it.

IOW, if there is a good concrete reason why it helps to teach the set of functions involved in a callchain to operate on an in-core instance of the index_state, passing an istate through the callchain is a very welcome change. If you do not do so, and then just replace $foo_cache(...) with $foo_index(&the_index, ...), that is an irritating and useless code churn that harms other topics in flight.