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

[Stefan Beller]

On Tue, Feb 20, 2018 at 11:55 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The step to take an object store would just add expressiveness
>> to the code, which may help in understanding what part of the code is
>> related to what other part of the code, so it may be a readability gain
>> on its own?
>
> It certainly would allow you to have a standalone object store that
> is not associated with *any* repository, but if we are not using
> such a layout, I doubt it would be a readability gain to add excess
> and unexercised expressiveness to the code.

So you favor v1?
Duy seems to be ok with v1 too if there is consensus that it is best
(or rather "if it makes Stefan's life hell, it's not worth doing.")[1].

I will try to resend that v1 shortly[2], as the only actual concern about the
code was where one struct was defined[3]. All other discussion was
meta-level, which direction to go.

Thanks for the clarification!
Stefan

[1] 
https://public-inbox.org/git/cacsjy8cpkese8atc_ewdnvknqyp9t6ebwkwcdzlhyafkh2b...@mail.gmail.com/
[2] https://public-inbox.org/git/20180213012241.187007-1-sbel...@google.com/
[3] https://public-inbox.org/git/20180213185120.ga108...@google.com/

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

[Junio C Hamano]

Stefan Beller  writes:

> The step to take an object store would just add expressiveness
> to the code, which may help in understanding what part of the code is
> related to what other part of the code, so it may be a readability gain
> on its own?

It certainly would allow you to have a standalone object store that
is not associated with *any* repository, but if we are not using
such a layout, I doubt it would be a readability gain to add excess
and unexercised expressiveness to the code.

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

[Stefan Beller]

On Tue, Feb 20, 2018 at 11:03 AM, Junio C Hamano  wrote:
> Jonathan Nieder  writes:
>
>> 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?
>
> Are we envisioning the future in which one repository has more than
> one object-store (I am counting an object store and its alternates
> that are pointed by its $GIT_OBJECT_DIRECTORY/info/alternates as a
> single logical "object store")?  If not, doing f(the_repository)
> migration, stopping there without f(the_repository->object_store)
> may perfectly be adequate, I would think.
>

I do not envision that, maybe Christian Couder does?
The partial clone world would be happy with just one object store
per repo, I would think.

The step to take an object store would just add expressiveness
to the code, which may help in understanding what part of the code is
related to what other part of the code, so it may be a readability gain
on its own?

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

[Stefan Beller]

On Tue, Feb 20, 2018 at 11:00 AM, Brandon Williams  wrote:
> On 02/20, Stefan Beller wrote:
>> On Fri, Feb 16, 2018 at 2:34 PM, Jonathan Nieder  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

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

[Junio C Hamano]

Jonathan Nieder  writes:

> 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?

Are we envisioning the future in which one repository has more than
one object-store (I am counting an object store and its alternates
that are pointed by its $GIT_OBJECT_DIRECTORY/info/alternates as a
single logical "object store")?  If not, doing f(the_repository)
migration, stopping there without f(the_repository->object_store)
may perfectly be adequate, I would think.

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

[Brandon Williams]

On 02/20, Stefan Beller wrote:
> On Fri, Feb 16, 2018 at 2:34 PM, Jonathan Nieder  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 which would make
it easier to write the patches (which most of them are already written)
and to review because you can use the #define trick to make some sort of
guarantees.

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).

-- 
Brandon Williams

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

[Stefan Beller]

On Fri, Feb 16, 2018 at 2:34 PM, Jonathan Nieder  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.

Stefan

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

[Jonathan Nieder]

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?

Thanks,
Jonathan