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

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

[Stefan Beller]

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.
  
Thanks,
Stefan

v1:
This is a real take on the first part of the recent RFC[1].

Jonathan Tan suggested[2] that "sha1_loose_object_info to handle arbitrary 
repositories"
might be a good breaking point for a first part at that RFC at patch 38.
This series is smaller and contains only 26 patches as the patches in the big
RFC were slightly out of order.

I developed this series partly by writing patches, but mostly by cherrypicking
from that RFC on top of current master. I noticed no external conflicts apart
from one addition to the repositories _INIT macro, which was easy to resolve.

Comments in the early range of that RFC were on 003 where Junio pointed out
that the coccinelle patch ought to be not in contrib/coccinelle, so I put it
in a sub directory there, as 'make coccicheck' doesn't traverse subdirs.

brian had a questoin on patch 25 in the RFC, but that seemed to resolve itself
without any suggestion to include into this series[3].

Duy suggested that we shall not use the repository blindly, but should carefully
examine whether to pass on an object store or the refstore or such[4], which
I agree with if it makes sense. This series unfortunately has an issue with that
as I would not want to pass down the `ignore_env` flag separately from the 
object
store, so I made all functions that only take the object store to have the raw
object store as the first parameter, and others using the full repository.

Eric Sunshine brought up memory leaks with the RFC, and I would think to
have plugged all holes.

[1] https://public-inbox.org/git/20180205235508.216277-1-sbel...@google.com/
[2] 
https://public-inbox.org/git/20180207143300.ce1c39ca07f6a0d64fe0e...@google.com/
[3] 
https://public-inbox.org/git/20180206011940.gd7...@genre.crustytoothpaste.net/
[4] 
https://public-inbox.org/git/cacsjy8cggekpx4czkyytsprj87uqvkzsol7fyt__p2dh_1l...@mail.gmail.com/

Thanks,
Stefan

Stefan Beller (16):
  repository: introduce raw object store field
  object-store: move alt_odb_list and alt_odb_tail to object store
  object-store: free alt_odb_list
  object-store: move packed_git and packed_git_mru to object store
  object-store: close all packs upon clearing the object store
  pack: move prepare_packed_git_run_once to object store
  pack: move approximate object count to object store
  sha1_file: add raw_object_store argument to alt_odb_usable
  sha1_file: allow link_alt_odb_entries to handle arbitrary object
stores
  sha1_file: allow prepare_alt_odb to handle arbitrary object stores
  sha1_file: allow sha1_file_name to handle arbitrary object stores
  sha1_file: allow stat_sha1_file to handle arbitrary object stores
  sha1_file: allow open_sha1_file to handle arbitrary object stores
  sha1_file: allow map_sha1_file_1 to handle arbitrary object stores
  sha1_file: allow map_sha1_file to handle arbitrary object stores
  sha1_file: allow sha1_loose_object_info to handle arbitrary object
stores

 builtin/am.c |   2 +-
 builtin/clone.c  |   2 +-
 builtin/count-objects.c  |   6 +-
 builtin/fetch.c  |   2 +-
 builtin/fsck.c   |  13 +++--
 builtin/gc.c |   4 +-
 builtin/grep.c   |   2 +-
 builtin/index-pack.c |   1 +
 builtin/merge.c  |   2 +-
 builtin/pack-objects.c   |  19 +++---
 builtin/pack-redundant.c |   6 +-
 builtin/receive-pack.c   |   3 +-
 cache.h  |  45 ++
 environment.c|   5 +-
 fast-import.c|   6 +-
 http-backend.c   |   6 +-
 http-push.c  |   1 +
 http-walker.c|   4 +-
 http.c   |   6 +-
 object-store.h   |  80 +
 object.c |  27 +
 pack-bitmap.c|   4 +-
 pack-check.c |   1 +
 pack-revindex.c  |   1 +
 packfile.c   |  64 ++--
 packfile.h   |   2 +-
 path.c   |   2 +-
 reachable.c  |   1 +
 repository.c |  22 +--
 repository.h |   7 ++-
 server-info.c|   6 +-
 sha1_file.c  | 123 +--
 sha1_name.c  |  11 ++--
 streaming.c  |   5 +-
 34 files changed, 314 insertions(+), 177 deletions(-)
 create mode 100644 object-store.h

-- 
2.16.1.291.g4437f3f132-goog