Re: [PATCHv2 00/16] Moving global state into the repository object (part 1)

From: Stefan Beller <sbeller@google.com>
Date: Feb 20, 2018, 19:03 UTC
Message-ID: <CAGZ79kZaDieXu+O6zA1W1pciMqa5_enjNXr9MOuzn=3oDSoU-w@mail.gmail.com>
In-Reply-To: <20180220190009.GA201604@google.com>

On Tue, Feb 20, 2018 at 11:00 AM, Brandon Williams <bmwill@google.com> wrote:

Show 49 quoted lines

> On 02/20, Stefan Beller wrote:
>> On Fri, Feb 16, 2018 at 2:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> > Hi,
>> >
>> > Stefan Beller wrote:
>> >
>> >> v2:
>> >> * duplicated the 'ignore_env' bit into the object store as well
>> >> * the #define trick no longer works as we do not have a "the_objectstore" global,
>> >>   which means there is just one patch per function that is converted.
>> >>   As this follows the same structure of the previous series, I am still confident
>> >>   there is no hidden dependencies to globals outside the object store in these
>> >>   converted functions.
>> >> * rebased on top of current master, resolving the merge conflicts.
>> >>   I think I used the list.h APIs right, but please double check.
>> >
>> > For what it's worth, I think I prefer v1.  I put some comments on why
>> > on patch 0 of v1 and would be interested in your thoughts on them
>> > (e.g. as a reply to that).  I also think that even if we want to
>> > switch to a style that passes around object_store separately from
>> > repository, it is easier to do the migration in two steps: first get
>> > rid of hidden dependencies on the_repository, then do the (simpler)
>> > automatic migration from
>> >
>> >  f(the_repository)
>> >
>> > to
>> >
>> >  f(the_repository->object_store)
>> >
>> > *afterwards*.
>> >
>> > Thoughts?
>>
>> I would prefer to not spend more time on these conversions.
>> If Duy and you would come to a conclusion to either pick this
>> or the previous version I would be happy.
>>
>> I do not see the benefit in splitting up the series even further and
>> do this multistep f(repo) -> f(object store), as the cost in potential
>> merge conflicts is too high. Note that brian just sent another object
>> id conversion series, also touching sha1_file.c, which I am sure will
>> produce merge conflicts for Junio.
>>
>> For the next part 2 and onwards I'd be happy to take either this
>> strategy or Duys strategy as requested.
>
> I think Jonathan is trying to point out that converting to f(repo) maybe
> easier than converting to f(repo->object_store) upfront

I agree.

> which would make
> it easier to write the patches (which most of them are already written)

true, but for this series we also have the conversion to f(object_store) written already.

> and to review because you can use the #define trick to make some sort of
> guarantees.

That is true, so it would be a tradeoff between reviewers and maintainers time?

> After we have successfully completed the migration to f(repo), then we
> can revisit the subsystems which want to have a clearer abstraction
> layer and make the jump to f(repo->object_store).

I would think we can take this series as-is and then move on with making f(repo) abstractions, eventually moving to f(specialized-subsystem) as those patches are not written yet (neither direct conversions, nor the repo trick; the patches I already have need adaption which takes enough time on its own.)

>
> --
> Brandon Williams