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

[Duy Nguyen]

On Tue, Feb 27, 2018 at 3:50 AM, Stefan Beller  wrote:
>> The natural thing to do is move these to raw_object_store too (and
>> repo_submodule_init needs to check submodule's config file). But one
>> may argue these should be per-process instead of per-repo though. I
>> don't know. But I thought I should mention this.
>
> For now a process and a repository is the same as git-gc or git-repack

I think you're thinking about the pack writing part, but these are
configuration for pack reading (aka "object store"). If you read a
blob from a submodule (e.g. git-grep), you'll use these configurations
at some point.

There are of course more configuration for pack writing (like
zlib_compression_level) which I deliberately avoided to mention
because I don't even know where they belong.

> doesn't know about the --recurse-submodules flag, yet.
> I wonder if we ever want to teach those commands the submodule
> recursion, because of the issue you bring up, which settings do we apply
> for a submodule? Traditionally we'd just have the command line override
> the configuration, which I don't know if it is a good idea for these settings.
-- 
Duy

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

[Stefan Beller]

On Sat, Feb 24, 2018 at 7:00 AM, Duy Nguyen  wrote:
> I notice that there are a few global state (or configuration rather)
> left after this: packed_git_window_size, packed_git_limit and
> delta_base_cache_limit. These can be changed by $GIT_DIR/config, but
> since it's still global, a submodule repository will use the same
> settings of its super repository. If $SUBMODULE/config changes any of
> these, they are ignored.

That sounds all packing related, which I plan on working on next.

> The natural thing to do is move these to raw_object_store too (and
> repo_submodule_init needs to check submodule's config file). But one
> may argue these should be per-process instead of per-repo though. I
> don't know. But I thought I should mention this.

For now a process and a repository is the same as git-gc or git-repack
doesn't know about the --recurse-submodules flag, yet.
I wonder if we ever want to teach those commands the submodule
recursion, because of the issue you bring up, which settings do we apply
for a submodule? Traditionally we'd just have the command line override
the configuration, which I don't know if it is a good idea for these settings.

Thanks,
Stefan

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

[Jonathan Tan]

On Fri, 23 Feb 2018 16:47:27 -0800
Stefan Beller  wrote:

> v4:
> * addressed feedback from the single patches (mostly nits)
> * rebased on latest master

The patches I looked at previously (patches 7, 15, 19, 22, and 24) look
good to me.

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

[Duy Nguyen]

I notice that there are a few global state (or configuration rather)
left after this: packed_git_window_size, packed_git_limit and
delta_base_cache_limit. These can be changed by $GIT_DIR/config, but
since it's still global, a submodule repository will use the same
settings of its super repository. If $SUBMODULE/config changes any of
these, they are ignored.

The natural thing to do is move these to raw_object_store too (and
repo_submodule_init needs to check submodule's config file). But one
may argue these should be per-process instead of per-repo though. I
don't know. But I thought I should mention this.
-- 
Duy

[PATCHv4 00/27] Moving global state into the repository object (part 1)

[Stefan Beller]

v4:
* addressed feedback from the single patches (mostly nits)
* rebased on latest master

v3:
* reverted back to use the repository for most of the functions
  (including unduplicating 'ignore_env')
* rebased on master again (I lost that state when doing v2, as
  I did both rebase as well as conversion to object store in one go)
* one additional patch, that moves the alternates related things to
  object-store.h, such that inclusion of cache.h (in object-store.h)
  is not required any more.

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

Jonathan Nieder (2):
  sha1_file: allow map_sha1_file_1 to handle arbitrary repositories
  sha1_file: allow sha1_loose_object_info to handle arbitrary
repositories

Stefan Beller (25):
  repository: introduce raw object store field
  object-store: migrate alternates struct and functions from cache.h
  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: add repository argument to link_alt_odb_entry
  sha1_file: add repository argument to read_info_alternates
  sha1_file: add repository argument to link_alt_odb_entries
  sha1_file: add repository argument to prepare_alt_odb
  sha1_file: allow link_alt_odb_entries to handle arbitrary repositories
  sha1_file: allow prepare_alt_odb to handle arbitrary repositories
  sha1_file: add repository argument to sha1_file_name
  sha1_file: add repository argument to stat_sha1_file
  sha1_file: add repository argument to open_sha1_file
  sha1_file: add repository argument to map_sha1_file_1
  sha1_file: add repository argument to map_sha1_file
  sha1_file: add repository argument to sha1_loose_object_info
  sha1_file: allow sha1_file_name to handle arbitrary repositories
  sha1_file: allow stat_sha1_file to handle arbitrary repositories
  sha1_file: allow open_sha1_file to handle arbitrary repositories
  sha1_file: allow map_sha1_file to handle arbitrary repositories

 builtin/am.c |   2 +-
 builtin/clone.c  |   2 +-
 builtin/count-objects.c  |   5 +-
 builtin/fetch.c  |   2 +-
 builtin/fsck.c   |  12 ++--
 builtin/gc.c |   3 +-
 builtin/grep.c   |   2 +-
 builtin/merge.c  |   2 +-
 builtin/pack-objects.c   |  20 ---
 builtin/pack-redundant.c |   5 +-