From: Derrick Stolee <stolee@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>, Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 00/11] Moving global state into the repository object (part 2)
Date: Wed, 28 Feb 2018 09:47:47 -0500 [thread overview]
Message-ID: <db9d8194-aeb0-92c2-aea0-1ec9fbbfd8ae@gmail.com> (raw)
In-Reply-To: <20180228021530.GA20625@duynguyen.dek-tpc.internal>
On 2/27/2018 9:15 PM, Duy Nguyen wrote:
> On Tue, Feb 27, 2018 at 05:05:57PM -0800, Stefan Beller wrote:
>> This applies on top of origin/sb/object-store and is the continuation of
>> that series, adding the repository as a context argument to functions.
>>
>> This series focusses on packfile handling, exposing (re)prepare_packed_git
>> and find_pack_entry to a repository argument.
> It looks good.
I agree.
> Looking at the full-series diff though, it makes me wonder if we
> should keep prepare_packed_git() private (i.e. how we initialize the
> object store, packfile included, is a private matter). How about
> something like this on top?
I think the get_packed_git() approach is cleaner than navigating
directly to the_repository->objects.packed_git that you expect to be
initialized by an earlier method call.
Thanks,
-Stolee
> -- 8< --
> Subject: [PATCH] packfile: keep raw_object_store.packed_git{,_mru} fields private
>
> These fields are initialized lazily via prepare_packed_git(). All
> access to these must call that function first but unless you know the
> implementation details, you may not see the connection.
>
> Keep that connection internal. These fields should only be accessed
> via get_packed_git() and get_packed_git_mru() outside packfile.c.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/count-objects.c | 4 +---
> builtin/fsck.c | 6 ++----
> builtin/gc.c | 3 +--
> builtin/pack-objects.c | 21 +++++++++++----------
> builtin/pack-redundant.c | 6 ++----
> fast-import.c | 7 ++-----
> http-backend.c | 5 ++---
> object-store.h | 4 ++--
> pack-bitmap.c | 3 +--
> packfile.c | 15 ++++++++++++++-
> packfile.h | 6 +++++-
> server-info.c | 5 ++---
> sha1_name.c | 6 ++----
> 13 files changed, 47 insertions(+), 44 deletions(-)
>
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index d480301763..088309945b 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -121,9 +121,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
> struct strbuf loose_buf = STRBUF_INIT;
> struct strbuf pack_buf = STRBUF_INIT;
> struct strbuf garbage_buf = STRBUF_INIT;
> - if (!the_repository->objects.packed_git)
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (!p->pack_local)
> continue;
> if (open_pack_index(p))
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 0a043a43c2..6d86f2581a 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -732,10 +732,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
> uint32_t total = 0, count = 0;
> struct progress *progress = NULL;
>
> - prepare_packed_git(the_repository);
> -
> if (show_progress) {
> - for (p = the_repository->objects.packed_git; p;
> + for (p = get_packed_git(the_repository); p;
> p = p->next) {
> if (open_pack_index(p))
> continue;
> @@ -744,7 +742,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>
> progress = start_progress(_("Checking objects"), total);
> }
> - for (p = the_repository->objects.packed_git; p;
> + for (p = get_packed_git(the_repository); p;
> p = p->next) {
> /* verify gives error messages itself */
> if (verify_pack(p, fsck_obj_buffer,
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 80d19c54d5..be63bec09c 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -173,8 +173,7 @@ static int too_many_packs(void)
> if (gc_auto_pack_limit <= 0)
> return 0;
>
> - prepare_packed_git(the_repository);
> - for (cnt = 0, p = the_repository->objects.packed_git; p; p = p->next) {
> + for (cnt = 0, p = get_packed_git(the_repository); p; p = p->next) {
> if (!p->pack_local)
> continue;
> if (p->pack_keep)
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 5e2590f882..a305f50100 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1026,7 +1026,7 @@ static int want_object_in_pack(const struct object_id *oid,
> if (want != -1)
> return want;
> }
> - list_for_each(pos, &the_repository->objects.packed_git_mru) {
> + list_for_each(pos, get_packed_git_mru(the_repository)) {
> struct packed_git *p = list_entry(pos, struct packed_git, mru);
> off_t offset;
>
> @@ -1045,7 +1045,7 @@ static int want_object_in_pack(const struct object_id *oid,
> want = want_found_object(exclude, p);
> if (!exclude && want > 0)
> list_move(&p->mru,
> - &the_repository->objects.packed_git_mru);
> + get_packed_git_mru(the_repository));
> if (want != -1)
> return want;
> }
> @@ -2674,7 +2674,7 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)
>
> memset(&in_pack, 0, sizeof(in_pack));
>
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> struct object_id oid;
> struct object *o;
>
> @@ -2737,8 +2737,10 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
> static struct packed_git *last_found = (void *)1;
> struct packed_git *p;
>
> - p = (last_found != (void *)1) ? last_found :
> - the_repository->objects.packed_git;
> + if (last_found != (void *)1)
> + p = last_found;
> + else
> + p = get_packed_git(the_repository);
>
> while (p) {
> if ((!p->pack_local || p->pack_keep) &&
> @@ -2747,7 +2749,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
> return 1;
> }
> if (p == last_found)
> - p = the_repository->objects.packed_git;
> + p = get_packed_git(the_repository);
> else
> p = p->next;
> if (p == last_found)
> @@ -2783,7 +2785,7 @@ static void loosen_unused_packed_objects(struct rev_info *revs)
> uint32_t i;
> struct object_id oid;
>
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (!p->pack_local || p->pack_keep)
> continue;
>
> @@ -3151,10 +3153,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> if (progress && all_progress_implied)
> progress = 2;
>
> - prepare_packed_git(the_repository);
> if (ignore_packed_keep) {
> struct packed_git *p;
> - for (p = the_repository->objects.packed_git; p; p = p->next)
> + for (p = get_packed_git(the_repository); p; p = p->next)
> if (p->pack_local && p->pack_keep)
> break;
> if (!p) /* no keep-able packs found */
> @@ -3167,7 +3168,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> * also covers non-local objects
> */
> struct packed_git *p;
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (!p->pack_local) {
> have_non_local_packs = 1;
> break;
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index 3b02f94248..02b5f0becc 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -572,7 +572,7 @@ static struct pack_list * add_pack(struct packed_git *p)
>
> static struct pack_list * add_pack_file(const char *filename)
> {
> - struct packed_git *p = the_repository->objects.packed_git;
> + struct packed_git *p = get_packed_git(the_repository);
>
> if (strlen(filename) < 40)
> die("Bad pack filename: %s", filename);
> @@ -587,7 +587,7 @@ static struct pack_list * add_pack_file(const char *filename)
>
> static void load_all(void)
> {
> - struct packed_git *p = the_repository->objects.packed_git;
> + struct packed_git *p = get_packed_git(the_repository);
>
> while (p) {
> add_pack(p);
> @@ -630,8 +630,6 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
> break;
> }
>
> - prepare_packed_git(the_repository);
> -
> if (load_all_packs)
> load_all();
> else
> diff --git a/fast-import.c b/fast-import.c
> index f2e255ed7e..041217eedf 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1111,8 +1111,7 @@ static int store_object(
> if (e->idx.offset) {
> duplicate_count_by_type[type]++;
> return 1;
> - } else if (find_sha1_pack(oid.hash,
> - the_repository->objects.packed_git)) {
> + } else if (find_sha1_pack(oid.hash, get_packed_git(the_repository))) {
> e->type = type;
> e->pack_id = MAX_PACK_ID;
> e->idx.offset = 1; /* just not zero! */
> @@ -1309,8 +1308,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
> duplicate_count_by_type[OBJ_BLOB]++;
> truncate_pack(&checkpoint);
>
> - } else if (find_sha1_pack(oid.hash,
> - the_repository->objects.packed_git)) {
> + } else if (find_sha1_pack(oid.hash, get_packed_git(the_repository))) {
> e->type = OBJ_BLOB;
> e->pack_id = MAX_PACK_ID;
> e->idx.offset = 1; /* just not zero! */
> @@ -3474,7 +3472,6 @@ int cmd_main(int argc, const char **argv)
> rc_free[i].next = &rc_free[i + 1];
> rc_free[cmd_save - 1].next = NULL;
>
> - prepare_packed_git(the_repository);
> start_packfile();
> set_die_routine(die_nicely);
> set_checkpoint_signal();
> diff --git a/http-backend.c b/http-backend.c
> index defa6ba350..22d2e1668e 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -518,14 +518,13 @@ static void get_info_packs(struct strbuf *hdr, char *arg)
> size_t cnt = 0;
>
> select_getanyfile(hdr);
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (p->pack_local)
> cnt++;
> }
>
> strbuf_grow(&buf, cnt * 53 + 2);
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (p->pack_local)
> strbuf_addf(&buf, "P %s\n", p->pack_name + objdirlen + 6);
> }
> diff --git a/object-store.h b/object-store.h
> index afe2f93459..636375753f 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -87,9 +87,9 @@ struct raw_object_store {
> */
> char *objectdir;
>
> - struct packed_git *packed_git;
> + struct packed_git *packed_git; /* private */
> /* A most-recently-used ordered version of the packed_git list. */
> - struct list_head packed_git_mru;
> + struct list_head packed_git_mru; /* private */
>
> struct alternate_object_database *alt_odb_list;
> struct alternate_object_database **alt_odb_tail;
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index bcc04bc45e..2a007b5539 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -335,8 +335,7 @@ static int open_pack_bitmap(void)
>
> assert(!bitmap_git.map && !bitmap_git.loaded);
>
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> if (open_pack_bitmap_1(p) == 0)
> ret = 0;
> }
> diff --git a/packfile.c b/packfile.c
> index d9065ccd32..ed26ab1b3b 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -802,6 +802,7 @@ static void prepare_packed_git_one(struct repository *r, char *objdir, int local
> strbuf_release(&path);
> }
>
> +static void prepare_packed_git(struct repository *r);
> /*
> * Give a fast, rough count of the number of objects in the repository. This
> * ignores loose objects completely. If you have a lot of them, then either
> @@ -882,7 +883,7 @@ static void prepare_packed_git_mru(struct repository *r)
> list_add_tail(&p->mru, &r->objects.packed_git_mru);
> }
>
> -void prepare_packed_git(struct repository *r)
> +static void prepare_packed_git(struct repository *r)
> {
> struct alternate_object_database *alt;
>
> @@ -897,6 +898,18 @@ void prepare_packed_git(struct repository *r)
> r->objects.packed_git_initialized = 1;
> }
>
> +struct packed_git *get_packed_git(struct repository *r)
> +{
> + prepare_packed_git(r);
> + return r->objects.packed_git;
> +}
> +
> +struct list_head *get_packed_git_mru(struct repository *r)
> +{
> + prepare_packed_git(r);
> + return &r->objects.packed_git_mru;
> +}
> +
> void reprepare_packed_git(struct repository *r)
> {
> r->objects.approximate_object_count_valid = 0;
> diff --git a/packfile.h b/packfile.h
> index 6f7b9e91d6..1903984e22 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -3,6 +3,9 @@
>
> #include "oidset.h"
>
> +struct list_head;
> +struct packed_git;
> +
> /*
> * Generate the filename to be used for a pack file with checksum "sha1" and
> * extension "ext". The result is written into the strbuf "buf", overwriting
> @@ -34,7 +37,8 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_
> #define PACKDIR_FILE_GARBAGE 4
> extern void (*report_garbage)(unsigned seen_bits, const char *path);
>
> -extern void prepare_packed_git(struct repository *r);
> +extern struct packed_git *get_packed_git(struct repository *r);
> +extern struct list_head *get_packed_git_mru(struct repository *r);
> extern void reprepare_packed_git(struct repository *r);
> extern void install_packed_git(struct repository *r, struct packed_git *pack);
>
> diff --git a/server-info.c b/server-info.c
> index f5e4d1cc89..af737acd24 100644
> --- a/server-info.c
> +++ b/server-info.c
> @@ -200,8 +200,7 @@ static void init_pack_info(const char *infofile, int force)
> objdir = get_object_directory();
> objdirlen = strlen(objdir);
>
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p; p = p->next) {
> /* we ignore things on alternate path since they are
> * not available to the pullers in general.
> */
> @@ -211,7 +210,7 @@ static void init_pack_info(const char *infofile, int force)
> }
> num_pack = i;
> info = xcalloc(num_pack, sizeof(struct pack_info *));
> - for (i = 0, p = the_repository->objects.packed_git; p; p = p->next) {
> + for (i = 0, p = get_packed_git(the_repository); p; p = p->next) {
> if (!p->pack_local)
> continue;
> info[i] = xcalloc(1, sizeof(struct pack_info));
> diff --git a/sha1_name.c b/sha1_name.c
> index 773322ac81..f25a0970ca 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -195,8 +195,7 @@ static void find_short_packed_object(struct disambiguate_state *ds)
> {
> struct packed_git *p;
>
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p && !ds->ambiguous;
> + for (p = get_packed_git(the_repository); p && !ds->ambiguous;
> p = p->next)
> unique_in_pack(p, ds);
> }
> @@ -566,8 +565,7 @@ static void find_abbrev_len_packed(struct min_abbrev_data *mad)
> {
> struct packed_git *p;
>
> - prepare_packed_git(the_repository);
> - for (p = the_repository->objects.packed_git; p; p = p->next)
> + for (p = get_packed_git(the_repository); p; p = p->next)
> find_abbrev_len_for_pack(p, mad);
> }
>
next prev parent reply other threads:[~2018-02-28 14:47 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-28 1:05 [PATCH 00/11] Moving global state into the repository object (part 2) Stefan Beller
2018-02-28 1:05 ` [PATCH 01/11] packfile: allow prepare_packed_git_mru to handle arbitrary repositories Stefan Beller
2018-02-28 1:05 ` [PATCH 02/11] packfile: allow rearrange_packed_git " Stefan Beller
2018-02-28 1:06 ` [PATCH 03/11] packfile: allow install_packed_git " Stefan Beller
2018-02-28 14:38 ` Derrick Stolee
2018-02-28 1:06 ` [PATCH 04/11] packfile: add repository argument to prepare_packed_git_one Stefan Beller
2018-02-28 1:06 ` [PATCH 05/11] packfile: add repository argument to prepare_packed_git Stefan Beller
2018-02-28 1:06 ` [PATCH 06/11] packfile: add repository argument to reprepare_packed_git Stefan Beller
2018-02-28 1:06 ` [PATCH 07/11] packfile: allow prepare_packed_git_one to handle arbitrary repositories Stefan Beller
2018-02-28 1:06 ` [PATCH 08/11] packfile: allow prepare_packed_git " Stefan Beller
2018-02-28 1:06 ` [PATCH 09/11] packfile: allow reprepare_packed_git " Stefan Beller
2018-02-28 1:06 ` [PATCH 10/11] packfile: add repository argument to find_pack_entry Stefan Beller
2018-02-28 1:06 ` [PATCH 11/11] packfile: allow find_pack_entry to handle arbitrary repositories Stefan Beller
2018-02-28 2:15 ` [PATCH 00/11] Moving global state into the repository object (part 2) Duy Nguyen
2018-02-28 14:47 ` Derrick Stolee [this message]
2018-02-28 17:59 ` Junio C Hamano
2018-02-28 18:59 ` Stefan Beller
2018-02-28 19:09 ` Junio C Hamano
2018-02-28 17:57 ` Junio C Hamano
2018-02-28 18:56 ` Stefan Beller
2018-02-28 19:19 ` Jonathan Nieder
2018-02-28 19:02 ` Junio C Hamano
2018-02-28 19:09 ` Stefan Beller
-- strict thread matches above, loose matches on Subject: below --
2018-03-03 2:54 Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=db9d8194-aeb0-92c2-aea0-1ec9fbbfd8ae@gmail.com \
--to=stolee@gmail.com \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.com \
--cc=sbeller@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).