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

On 06/29, Junio C Hamano wrote:

Stefan Beller [off-list ref] writes:

quoted

On Fri, Jun 29, 2018 at 11:03 AM Junio C Hamano [off-list ref] wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

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.

I 100% think that we need to continue these refactorings with both the
object store as well as with the_index (removing the index macros and
removing the dependency on global state).  The whole compat macros most
definitely was a failed experiment as we still haven't rid the code
base of them yet.  Having two APIs which can be used interchangeably
from different pieces of library code which have very different
semantics is _very_ difficult for contributors to reason about and work
around.  Especially when one API relies on implied global state while
the other does not.  (If global state isn't involved, as in the char[20]
-> OID work, then having two APIs is 'ok' so long as you're working
towards converging to one API).

Having clean APIs makes it incredible easy to reason about sections of
code, reason about multi-threading, and implement newer features in a
sane manner (talking about submodules here).  I implemented submodule
support for grep twice, once using a multi-process paradigm because we
could not have two repositories open in a single process and then an
in-process one once a minimum viable solution for opening multiple
repositories in a single process was created.  Being able to open up a
submodule repository in-process made things infinitely simpler to reason
about as well as made it much easier to handle errors. This work needs
to continue if we want to improve the submodule experience in git.

Yes this might mean there are a few more conflicts when merging a series
(because of the scope of these refactorings) but it is well worth it
given what the end-state will look like.

-- 
Brandon Williams