From: Jeff King <peff@peff.net>
Date: Sat, 10 Jun 2017 06:07:12 GMT
Subject: Re: [PATCH v2 00/32] repository object
Message-ID: <20170610060712.foqre5fscaxu3tnx@sigill.intra.peff.net>
In-Reply-To: <20170609174034.61889ae8@twelve2.svl.corp.google.com>

On Fri, Jun 09, 2017 at 05:40:34PM -0700, Jonathan Tan wrote:

> Before I get into the details, I have some questions:
> 
> 1. I am concerned that "struct repository" will end up growing without
> bounds as we store more and more repo-specific concerns in it. Could it
> be restricted to just the fields populated by repo_init()?
> repo_read_index() will then return the index itself, instead of using
> "struct repository" as a cache. This means that code using
> repo_read_index() will need to maintain its own variable holding the
> returned index, but that is likely a positive - it's better for code to
> just pass around the specific thing needed between functions anyway, as
> opposed to passing a giant "struct repository" (which partially defeats
> the purpose of eliminating the usage of globals).

I think the repository object has to become a kitchen sink of sorts,
because we have tons of global variables representing repo-wide config.
ls-files doesn't respect a lot of config, but what should, e.g.:

  git config core.quotepath true
  git -C submodule config core.quotepath false
  git ls-files --recurse-submodules

do?  Right now, with a separate process, we respect the submodule
version of the config. But in a single process[1] we'd need one copy of
the quote_path_fully variable for each repo object. It's tempting for
this case to say that core.quotepath from the super-project should just
take precedence, as that's where the command is issued from (and why the
heck would anybody have per-repo settings for this anyway?). But I
suspect as we get into more complicated commands that there are likely
to be config variables that are important to match to each repo.

I do agree that "pass just what the sub-function needs" is a good rule
of thumb. But the reason that these are globals in the first place is
that there are a ton of them, and they are used at the lowest levels of
call chains. So I have a feeling that we're always going to need some
big object to hold all that context when doing multi-repo operations in
a single process.

For config, in theory that could be a big "config_set" object, but
that's not quite how we treat our config. We usually parse it once into
actual variables. So really you end up with a big parsed-config object
that gets passed around, I'd think.

-Peff

[1] I wanted to see how Brandon's series behaved for this quotepath
    case, but unfortunately I couldn't get it to work in even a simple
    case.  :(

      $ git ls-files --recurse-submodules
      fatal: index file corrupt