Re: [PATCH 44/44] packfile: keep prepare_packed_git() private
On 03/03, Nguyễn Thái Ngọc Duy wrote: > The reason callers have to call this is to make sure either packed_git > or packed_git_mru pointers are initialized since we don't do that by > default. Sometimes it's hard to see this connection between where the > function is called and where packed_git pointer is used (sometimes in > separate functions). > > Keep this dependency internal because now all access to packed_git and > packed_git_mru must go through get_xxx() wrappers. Ahh now I understand the rational for trying to make it "private". > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > builtin/count-objects.c | 3 +-- > builtin/fsck.c | 2 -- > builtin/gc.c | 1 - > builtin/pack-objects.c | 1 - > builtin/pack-redundant.c | 2 -- > fast-import.c| 1 - > http-backend.c | 1 - > pack-bitmap.c| 1 - > packfile.c | 5 - > packfile.h | 1 - > server-info.c| 1 - > sha1_name.c | 2 -- > 12 files changed, 5 insertions(+), 16 deletions(-) > > diff --git a/builtin/count-objects.c b/builtin/count-objects.c > index 2793c98ed3..ee6ae35244 100644 > --- a/builtin/count-objects.c > +++ b/builtin/count-objects.c > @@ -121,8 +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 (!get_packed_git(the_repository)) > - prepare_packed_git(the_repository); > + > for (p = get_packed_git(the_repository); p; p = p->next) { > if (!p->pack_local) > continue; > diff --git a/builtin/fsck.c b/builtin/fsck.c > index 7a3e323e9e..9911c52bc8 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -726,8 +726,6 @@ 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 = get_packed_git(the_repository); p; >p = p->next) { > diff --git a/builtin/gc.c b/builtin/gc.c > index 560d58daec..be63bec09c 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -173,7 +173,6 @@ static int too_many_packs(void) > if (gc_auto_pack_limit <= 0) > return 0; > > - prepare_packed_git(the_repository); > for (cnt = 0, p = get_packed_git(the_repository); p; p = p->next) { > if (!p->pack_local) > continue; > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 6020a7e230..435f091a69 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -3151,7 +3151,6 @@ 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 = get_packed_git(the_repository); p; p = p->next) > diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c > index bf42e164eb..02b5f0becc 100644 > --- a/builtin/pack-redundant.c > +++ b/builtin/pack-redundant.c > @@ -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 985eb2eccc..2298bfcdfd 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -3472,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 659ddfb5f1..22d2e1668e 100644 > --- a/http-backend.c > +++ b/http-backend.c > @@ -518,7 +518,6 @@ static void get_info_packs(struct strbuf *hdr, char *arg) > size_t cnt = 0; > > select_getanyfile(hdr); > - prepare_packed_git(the_repository); > for (p = get_packed_git(the_repository); p; p = p->next) { > if (p->pack_local) > cnt++; > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 01c9cd1642..2a007b5539 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -335,7 +335,6 @@ static int open_pack_bitmap(void) > > assert(!bitmap_git.map && !bitmap_git.loaded); > > - prepare_packed_git(the_repository); > for (p = get_packed_git(the_repository); p; p = p->next) { > if (op
Re: [PATCH 44/44] packfile: keep prepare_packed_git() private
On Sat, 3 Mar 2018 18:36:37 +0700 Nguyễn Thái Ngọc Duy wrote: > The reason callers have to call this is to make sure either packed_git > or packed_git_mru pointers are initialized since we don't do that by > default. Sometimes it's hard to see this connection between where the > function is called and where packed_git pointer is used (sometimes in > separate functions). > > Keep this dependency internal because now all access to packed_git and > packed_git_mru must go through get_xxx() wrappers. > > Signed-off-by: Nguyễn Thái Ngọc Duy The patches up to and including this one look good. I also see that the question I asked in patch 10 about lazily initializing some fields is answered here. If we're planning to avoid making the user call prepare_packed_git() (which I agree with), I think we also need to ensure that we always use the get_xxx() wrapper whenever we access objects.packed_git. Currently, there are still some functions in packfile.c that do not do so (notably for_each_packed_object()). Could these be changed too? (This should not be too difficult for you to do on your own, but I can send a fixup patch if you want.)
[PATCH 44/44] packfile: keep prepare_packed_git() private
The reason callers have to call this is to make sure either packed_git or packed_git_mru pointers are initialized since we don't do that by default. Sometimes it's hard to see this connection between where the function is called and where packed_git pointer is used (sometimes in separate functions). Keep this dependency internal because now all access to packed_git and packed_git_mru must go through get_xxx() wrappers. Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/count-objects.c | 3 +-- builtin/fsck.c | 2 -- builtin/gc.c | 1 - builtin/pack-objects.c | 1 - builtin/pack-redundant.c | 2 -- fast-import.c| 1 - http-backend.c | 1 - pack-bitmap.c| 1 - packfile.c | 5 - packfile.h | 1 - server-info.c| 1 - sha1_name.c | 2 -- 12 files changed, 5 insertions(+), 16 deletions(-) diff --git a/builtin/count-objects.c b/builtin/count-objects.c index 2793c98ed3..ee6ae35244 100644 --- a/builtin/count-objects.c +++ b/builtin/count-objects.c @@ -121,8 +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 (!get_packed_git(the_repository)) - prepare_packed_git(the_repository); + for (p = get_packed_git(the_repository); p; p = p->next) { if (!p->pack_local) continue; diff --git a/builtin/fsck.c b/builtin/fsck.c index 7a3e323e9e..9911c52bc8 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -726,8 +726,6 @@ 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 = get_packed_git(the_repository); p; p = p->next) { diff --git a/builtin/gc.c b/builtin/gc.c index 560d58daec..be63bec09c 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -173,7 +173,6 @@ static int too_many_packs(void) if (gc_auto_pack_limit <= 0) return 0; - prepare_packed_git(the_repository); for (cnt = 0, p = get_packed_git(the_repository); p; p = p->next) { if (!p->pack_local) continue; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 6020a7e230..435f091a69 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3151,7 +3151,6 @@ 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 = get_packed_git(the_repository); p; p = p->next) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index bf42e164eb..02b5f0becc 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -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 985eb2eccc..2298bfcdfd 100644 --- a/fast-import.c +++ b/fast-import.c @@ -3472,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 659ddfb5f1..22d2e1668e 100644 --- a/http-backend.c +++ b/http-backend.c @@ -518,7 +518,6 @@ static void get_info_packs(struct strbuf *hdr, char *arg) size_t cnt = 0; select_getanyfile(hdr); - prepare_packed_git(the_repository); for (p = get_packed_git(the_repository); p; p = p->next) { if (p->pack_local) cnt++; diff --git a/pack-bitmap.c b/pack-bitmap.c index 01c9cd1642..2a007b5539 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -335,7 +335,6 @@ static int open_pack_bitmap(void) assert(!bitmap_git.map && !bitmap_git.loaded); - prepare_packed_git(the_repository); 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 bafe81544d..773cd99a13 100644 --- a/packfile.c +++ b/packfile.c @@ -802,6 +802,7 @@ static void prepare_packed_git_one(struct repository *r, char *objdir, int local