Re: [RFC PATCH 000/194] Moving global state into the repository object

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3390 bytes --]

On Mon, Feb 05, 2018 at 03:51:54PM -0800, Stefan Beller wrote:
> This series moves a lot of global state into the repository struct.
> It applies on top of 2512f15446149235156528dafbe75930c712b29e (2.16.0)
> It can be found at https://github.com/stefanbeller/git/tree/object-store
> 
> Motivation for this series:
> * Easier to reason about code when all state is stored in one place,
>   for example for multithreading
> * Easier to move to processing submodules in-process instead of
>   calling a processes for the submodule handling.
>   The last patch demonstrates this.

Of course, I like performance improvements (who doesn't?), but I really
like the way this series gets rid of various global variables.  I much
prefer to have fewer globals when possible.

> This is why this series tries to be reviewer friendly by utilizing
> machine assistance as much as possible. There are 3 types of patches
> in this series:
> 
> (A) <area>: add repository argument to <function_foo>
>   This sort of patch is just adding the repository as an argument to that
>   function. It assumes the given repository is `the_repository` and has
>   a compile time check for that assertion using a preprocessor trick.
>   As did a compile check after each commit of the series, I don't expect
>   the review burden on these patches to be high. Review on these patches
>   is mostly checking for formatting errors or if I sneak in malicious
>   code -- if you're inclined to believe that.
> 
> (B) <area>: allow <function_foo> to handle arbitrary repositories
>   This sort of patch is touching code inside the given function only,
>   usually replacing all occurrences of `the_repository` by the argument
>   `r`. Here the review is critical: Did I miss any function that relies
>   on state of `the_repository` ?
>   This series was developed by converting all functions of
>   packfile/sha1-file/commit/etc using (A); after the demo patch
>   was possible, all patches that did (A), but not (B) were deleted.
>   Therefore I was confident at time of writing that patch that
>   the conversion of a function which doesn't take a repository argument
>   is okay.
> 
> (C) other patches, such as moving code around,
>     demoing this series in the last patch
> 
> This approach was chosen as I think it is review friendly, despite the
> huge number of patches.

I very much appreciate this approach.  It made reviewing things much
nicer.

> A weakness of this approach is the buildup of a large series, which ignores
> the ongoing development. Rebasing that series turned out to be ok, but merging
> it with confidence is an issue.

That's my concern as well.  This necessarily conflicts with some of the
object_id and the_hash_algo work, but rebasing work on top of it
shouldn't be too awful once it's ready to be picked up.  In fact, long
term, it makes that work easier, so I'm all for it.

I'm sure Junio will have thoughts on the size of the series and
potential conflicts in flight, but I'll let him articulate those.  The
series could in theory be split into pieces the way you've done it: it
would just be less convenient to work with some of the functions in the
mean time.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]