git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Brandon Williams <bmwill@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Stefan Beller <sbeller@google.com>, git <git@vger.kernel.org>
Subject: Re: [PATCH v3 00/32] object-store: lookup_commit
Date: Fri, 29 Jun 2018 15:20:04 -0700
Message-ID: <20180629222004.GA197765@google.com> (raw)
In-Reply-To: <xmqqvaa1tme8.fsf@gitster-ct.c.googlers.com>

On 06/29, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> > 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.

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

  reply index

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-29  1:21 Stefan Beller
2018-06-29  1:21 ` [PATCH v3 01/32] object: add repository argument to parse_object Stefan Beller
2018-06-29  1:21 ` [PATCH v3 02/32] object: add repository argument to lookup_object Stefan Beller
2018-06-29  1:21 ` [PATCH v3 03/32] object: add repository argument to parse_object_buffer Stefan Beller
2018-06-29  1:21 ` [PATCH v3 04/32] object: add repository argument to object_as_type Stefan Beller
2018-06-29  1:21 ` [PATCH v3 05/32] blob: add repository argument to lookup_blob Stefan Beller
2018-06-29  1:21 ` [PATCH v3 06/32] tree: add repository argument to lookup_tree Stefan Beller
2018-06-29  1:21 ` [PATCH v3 07/32] commit: add repository argument to lookup_commit_reference_gently Stefan Beller
2018-06-29  1:21 ` [PATCH v3 08/32] commit: add repository argument to lookup_commit_reference Stefan Beller
2018-06-29  1:21 ` [PATCH v3 09/32] commit: add repository argument to lookup_commit Stefan Beller
2018-06-29 14:46   ` Derrick Stolee
2018-06-29  1:22 ` [PATCH v3 10/32] commit: add repository argument to parse_commit_buffer Stefan Beller
2018-06-29 14:49   ` Derrick Stolee
2018-06-29  1:22 ` [PATCH v3 11/32] commit: add repository argument to set_commit_buffer Stefan Beller
2018-06-29  1:22 ` [PATCH v3 12/32] commit: add repository argument to get_cached_commit_buffer Stefan Beller
2018-06-29  1:22 ` [PATCH v3 13/32] tag: add repository argument to lookup_tag Stefan Beller
2018-06-29  1:22 ` [PATCH v3 14/32] tag: add repository argument to parse_tag_buffer Stefan Beller
2018-06-29  1:22 ` [PATCH v3 15/32] tag: add repository argument to deref_tag Stefan Beller
2018-06-29  1:22 ` [PATCH v3 16/32] object: allow object_as_type to handle arbitrary repositories Stefan Beller
2018-06-29  1:22 ` [PATCH v3 17/32] object: allow lookup_object " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 18/32] blob: allow lookup_blob " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 19/32] tree: allow lookup_tree " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 20/32] commit: allow lookup_commit " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 21/32] tag: allow lookup_tag " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 22/32] tag: allow parse_tag_buffer " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 23/32] commit.c: allow parse_commit_buffer " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 24/32] commit-slabs: remove realloc counter outside of slab struct Stefan Beller
2018-06-29  1:22 ` [PATCH v3 25/32] commit.c: migrate the commit buffer to the parsed object store Stefan Beller
2018-06-29  1:22 ` [PATCH v3 26/32] commit.c: allow set_commit_buffer to handle arbitrary repositories Stefan Beller
2018-06-29  1:22 ` [PATCH v3 27/32] commit.c: allow get_cached_commit_buffer " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 28/32] object.c: allow parse_object_buffer " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 29/32] object.c: allow parse_object " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 30/32] tag.c: allow deref_tag " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 31/32] commit.c: allow lookup_commit_reference_gently " Stefan Beller
2018-06-29  1:22 ` [PATCH v3 32/32] commit.c: allow lookup_commit_reference " Stefan Beller
2018-06-29 14:53 ` [PATCH v3 00/32] object-store: lookup_commit Derrick Stolee
2018-06-29 17:57 ` Junio C Hamano
2018-06-29 18:03   ` Junio C Hamano
2018-06-29 18:34     ` Stefan Beller
2018-06-29 19:37       ` Junio C Hamano
2018-06-29 22:20         ` Brandon Williams [this message]
2018-07-03 18:13           ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180629222004.GA197765@google.com \
    --to=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox