Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Brandon Williams]

On 03/23, Duy Nguyen wrote:
> On Fri, Mar 23, 2018 at 6:03 PM, Duy Nguyen  wrote:
> > On Wed, Mar 21, 2018 at 11:18 PM, Brandon Williams  
> > wrote:
> >> You're marking packed_git
> >> as "private"...well C has no notion of private vs public fields in a
> >> struct so it might be difficult to keep that convention, it also took me
> >> a second to realize that it was only in the scope of packfile.c where it
> >> was ok to reference it directly.  Maybe it'll be ok?  If we really
> >> wanted something to be private we'd need it to be an opaque type
> >> instead, which may be out of the scope of this code refactor.
> >
> > It's true C syntax does not support private/public scoping, but it
> > does not mean we must avoid that. Python has "private convention" with
> > the underscore prefix, no special support from the language either.
> > Yes having compiler support to enforce scoping is nice, but I think we
> > can still live without it (Go devs live fine without "const"
> > arguments, e.g.)
> >
> > So I'm going to make it clearer that these fields are not supposed to
> > be accessed outside packfile.c
> 
> I'm not counting out the making these fields completely opaque of
> course. And with your suggestion of not embedding raw_object_store to
> repository, that's actually possible to do. But I'm still not doing it
> now :) The series is getting long and extending its scope will drag it
> even longer (in terms of both time and number of patches)

Oh of course. I'm a big proponent of taking small steps, so definitely
avoid scope creep and hopefully this series can land quicker so we have
a better base to work with to make some of those other improvements if
we still want to down the road.

-- 
Brandon Williams

Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Duy Nguyen]

On Fri, Mar 23, 2018 at 6:03 PM, Duy Nguyen  wrote:
> On Wed, Mar 21, 2018 at 11:18 PM, Brandon Williams  wrote:
>> You're marking packed_git
>> as "private"...well C has no notion of private vs public fields in a
>> struct so it might be difficult to keep that convention, it also took me
>> a second to realize that it was only in the scope of packfile.c where it
>> was ok to reference it directly.  Maybe it'll be ok?  If we really
>> wanted something to be private we'd need it to be an opaque type
>> instead, which may be out of the scope of this code refactor.
>
> It's true C syntax does not support private/public scoping, but it
> does not mean we must avoid that. Python has "private convention" with
> the underscore prefix, no special support from the language either.
> Yes having compiler support to enforce scoping is nice, but I think we
> can still live without it (Go devs live fine without "const"
> arguments, e.g.)
>
> So I'm going to make it clearer that these fields are not supposed to
> be accessed outside packfile.c

I'm not counting out the making these fields completely opaque of
course. And with your suggestion of not embedding raw_object_store to
repository, that's actually possible to do. But I'm still not doing it
now :) The series is getting long and extending its scope will drag it
even longer (in terms of both time and number of patches)
-- 
Duy

Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Duy Nguyen]

On Wed, Mar 21, 2018 at 11:18 PM, Brandon Williams  wrote:
> You're marking packed_git
> as "private"...well C has no notion of private vs public fields in a
> struct so it might be difficult to keep that convention, it also took me
> a second to realize that it was only in the scope of packfile.c where it
> was ok to reference it directly.  Maybe it'll be ok?  If we really
> wanted something to be private we'd need it to be an opaque type
> instead, which may be out of the scope of this code refactor.

It's true C syntax does not support private/public scoping, but it
does not mean we must avoid that. Python has "private convention" with
the underscore prefix, no special support from the language either.
Yes having compiler support to enforce scoping is nice, but I think we
can still live without it (Go devs live fine without "const"
arguments, e.g.)

So I'm going to make it clearer that these fields are not supposed to
be accessed outside packfile.c
-- 
Duy

Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Duy Nguyen]

On Mon, Mar 19, 2018 at 8:39 PM, Jonathan Tan  wrote:
> On Sat,  3 Mar 2018 18:36:03 +0700
> Nguyễn Thái Ngọc Duy   wrote:
>
>> From: Stefan Beller 
>>
>> In a process with multiple repositories open, packfile accessors
>> should be associated to a single repository and not shared globally.
>> Move packed_git and packed_git_mru into the_repository and adjust
>> callers to reflect this.
>>
>> [nd: while at there, wrap access to these two fields in get_packed_git()
>> and get_packed_git_mru(). This allows us to lazily initialize these
>> fields without caller doing that explicitly]
>
> The patches up to this one look good. (I didn't reply for each
> individual one to avoid unnecessarily sending messages to the list.)
>
> About this patch: will lazily initializing these fields be done in a
> later patch?

Yes.


>> Patch generated by
>>
>>  1. Moving the struct packed_git declaration to object-store.h
>> and packed_git, packed_git_mru globals to struct object_store.
>>
>>  2. Apply the following semantic patch to adjust callers:
>> @@ @@
>> - packed_git
>> + the_repository->objects.packed_git
>>
>> @@ @@
>> - packed_git_mru
>> + the_repository->objects.packed_git_mru
>
> This doesn't seem up-to-date - they are being replaced with a function
> call, not a field access. I would be OK with just removing this "Patch
> generated by" section.

I'll just drop this part. I'm manually updating the series anyway.


> [snip remainder of "Patch generated by" section]
>
>> @@ -246,7 +244,7 @@ static int unuse_one_window(struct packed_git *current)
>>
>>   if (current)
>>   scan_windows(current, _p, _w, _l);
>> - for (p = packed_git; p; p = p->next)
>> + for (p = the_repository->objects.packed_git; p; p = p->next)
>>   scan_windows(p, _p, _w, _l);
>>   if (lru_p) {
>>   munmap(lru_w->base, lru_w->len);
>
> Here (and elsewhere), "the_repository->objects.packed_git" instead of
> "get_packed_git" is still used.


Yeah. I saw your other comment about the not converting in packfile.c
too. My view is, packfile has intimate knowledge about pack
manipulation and we don't need to try to "protect" from misuse of
packed_git pointer without prepare_packed_git. People how modify this
code probably know that by heart at this point. And we can't avoid it
completely anyway because it will have access to the static function
prepare_packed_git.
-- 
Duy

Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Brandon Williams]

On 03/03, Nguyễn Thái Ngọc Duy wrote:
> From: Stefan Beller 
> 
> In a process with multiple repositories open, packfile accessors
> should be associated to a single repository and not shared globally.
> Move packed_git and packed_git_mru into the_repository and adjust
> callers to reflect this.
> 
> [nd: while at there, wrap access to these two fields in get_packed_git()
> and get_packed_git_mru(). This allows us to lazily initialize these
> fields without caller doing that explicitly]
> 
> Patch generated by
> 
>  1. Moving the struct packed_git declaration to object-store.h
> and packed_git, packed_git_mru globals to struct object_store.
> 
>  2. Apply the following semantic patch to adjust callers:
> @@ @@
> - packed_git
> + the_repository->objects.packed_git
> 
> @@ @@
> - packed_git_mru
> + the_repository->objects.packed_git_mru

As Jonathan mentioned, this should probably be taken out of the commit
message because it doesn't reflect what the code actually does.  What it
actually does took me a second to figure out.  You're marking packed_git
as "private"...well C has no notion of private vs public fields in a
struct so it might be difficult to keep that convention, it also took me
a second to realize that it was only in the scope of packfile.c where it
was ok to reference it directly.  Maybe it'll be ok?  If we really
wanted something to be private we'd need it to be an opaque type
instead, which may be out of the scope of this code refactor.

> 
>  3. Applying line wrapping fixes from "make style" to break the
> resulting long lines.
> 
>  4. Adding missing #includes of repository.h where needed.
> 
>  5. As the packfiles are now owned by an objectstore, which is ephemeral
> unlike globals, we introduce memory leaks. So address them in
> raw_object_store_clear(). Defer freeing packed_git to the next
> patch due to the size of this patch.
> 
> Signed-off-by: Stefan Beller 
> Signed-off-by: Jonathan Nieder 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/count-objects.c  |  5 ++--
>  builtin/fsck.c   |  6 +++--
>  builtin/gc.c |  3 ++-
>  builtin/pack-objects.c   | 20 
>  builtin/pack-redundant.c |  5 ++--
>  cache.h  | 29 
>  fast-import.c|  7 --
>  http-backend.c   |  5 ++--
>  object-store.h   | 28 +++
>  object.c |  7 ++
>  pack-bitmap.c|  3 ++-
>  packfile.c   | 49 
>  packfile.h   |  3 +++
>  server-info.c|  5 ++--
>  sha1_name.c  |  5 ++--
>  15 files changed, 107 insertions(+), 73 deletions(-)
> 
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index 33343818c8..5c7c3c6ae3 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -7,6 +7,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "dir.h"
> +#include "repository.h"
>  #include "builtin.h"
>  #include "parse-options.h"
>  #include "quote.h"
> @@ -120,9 +121,9 @@ 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 (!packed_git)
> + if (!get_packed_git(the_repository))
>   prepare_packed_git();
> - for (p = 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 b284a3a74e..7aca9699f6 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -729,7 +729,8 @@ int cmd_fsck(int argc, const char **argv, const char 
> *prefix)
>   prepare_packed_git();
>  
>   if (show_progress) {
> - for (p = packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p;
> +  p = p->next) {
>   if (open_pack_index(p))
>   continue;
>   total += p->num_objects;
> @@ -737,7 +738,8 @@ int cmd_fsck(int argc, const char **argv, const char 
> *prefix)
>  
>   progress = start_progress(_("Checking 
> objects"), total);
>   }
> - for (p = packed_git; p; p = p->next) {
> + for (p = get_packed_git(the_repository); p;
> +  p = p->next) {
>   /* verify gives error messages itself 

Re: [PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Jonathan Tan]

On Sat,  3 Mar 2018 18:36:03 +0700
Nguyễn Thái Ngọc Duy   wrote:

> From: Stefan Beller 
> 
> In a process with multiple repositories open, packfile accessors
> should be associated to a single repository and not shared globally.
> Move packed_git and packed_git_mru into the_repository and adjust
> callers to reflect this.
> 
> [nd: while at there, wrap access to these two fields in get_packed_git()
> and get_packed_git_mru(). This allows us to lazily initialize these
> fields without caller doing that explicitly]

The patches up to this one look good. (I didn't reply for each
individual one to avoid unnecessarily sending messages to the list.)

About this patch: will lazily initializing these fields be done in a
later patch?

> Patch generated by
> 
>  1. Moving the struct packed_git declaration to object-store.h
> and packed_git, packed_git_mru globals to struct object_store.
> 
>  2. Apply the following semantic patch to adjust callers:
> @@ @@
> - packed_git
> + the_repository->objects.packed_git
> 
> @@ @@
> - packed_git_mru
> + the_repository->objects.packed_git_mru

This doesn't seem up-to-date - they are being replaced with a function
call, not a field access. I would be OK with just removing this "Patch
generated by" section.

[snip remainder of "Patch generated by" section]

> @@ -246,7 +244,7 @@ static int unuse_one_window(struct packed_git *current)
>  
>   if (current)
>   scan_windows(current, _p, _w, _l);
> - for (p = packed_git; p; p = p->next)
> + for (p = the_repository->objects.packed_git; p; p = p->next)
>   scan_windows(p, _p, _w, _l);
>   if (lru_p) {
>   munmap(lru_w->base, lru_w->len);

Here (and elsewhere), "the_repository->objects.packed_git" instead of
"get_packed_git" is still used.

[PATCH 10/44] object-store: move packed_git and packed_git_mru to object store

[Nguyễn Thái Ngọc Duy]

From: Stefan Beller 

In a process with multiple repositories open, packfile accessors
should be associated to a single repository and not shared globally.
Move packed_git and packed_git_mru into the_repository and adjust
callers to reflect this.

[nd: while at there, wrap access to these two fields in get_packed_git()
and get_packed_git_mru(). This allows us to lazily initialize these
fields without caller doing that explicitly]

Patch generated by

 1. Moving the struct packed_git declaration to object-store.h
and packed_git, packed_git_mru globals to struct object_store.

 2. Apply the following semantic patch to adjust callers:
@@ @@
- packed_git
+ the_repository->objects.packed_git

@@ @@
- packed_git_mru
+ the_repository->objects.packed_git_mru

 3. Applying line wrapping fixes from "make style" to break the
resulting long lines.

 4. Adding missing #includes of repository.h where needed.

 5. As the packfiles are now owned by an objectstore, which is ephemeral
unlike globals, we introduce memory leaks. So address them in
raw_object_store_clear(). Defer freeing packed_git to the next
patch due to the size of this patch.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/count-objects.c  |  5 ++--
 builtin/fsck.c   |  6 +++--
 builtin/gc.c |  3 ++-
 builtin/pack-objects.c   | 20 
 builtin/pack-redundant.c |  5 ++--
 cache.h  | 29 
 fast-import.c|  7 --
 http-backend.c   |  5 ++--
 object-store.h   | 28 +++
 object.c |  7 ++
 pack-bitmap.c|  3 ++-
 packfile.c   | 49 
 packfile.h   |  3 +++
 server-info.c|  5 ++--
 sha1_name.c  |  5 ++--
 15 files changed, 107 insertions(+), 73 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 33343818c8..5c7c3c6ae3 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -7,6 +7,7 @@
 #include "cache.h"
 #include "config.h"
 #include "dir.h"
+#include "repository.h"
 #include "builtin.h"
 #include "parse-options.h"
 #include "quote.h"
@@ -120,9 +121,9 @@ 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 (!packed_git)
+   if (!get_packed_git(the_repository))
prepare_packed_git();
-   for (p = 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 b284a3a74e..7aca9699f6 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -729,7 +729,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
prepare_packed_git();
 
if (show_progress) {
-   for (p = packed_git; p; p = p->next) {
+   for (p = get_packed_git(the_repository); p;
+p = p->next) {
if (open_pack_index(p))
continue;
total += p->num_objects;
@@ -737,7 +738,8 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
 
progress = start_progress(_("Checking 
objects"), total);
}
-   for (p = packed_git; p; p = p->next) {
+   for (p = get_packed_git(the_repository); p;
+p = p->next) {
/* verify gives error messages itself */
if (verify_pack(p, fsck_obj_buffer,
progress, count))
diff --git a/builtin/gc.c b/builtin/gc.c
index 77fa720bd0..dd30067ac1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -11,6 +11,7 @@
  */
 
 #include "builtin.h"
+#include "repository.h"
 #include "config.h"
 #include "tempfile.h"
 #include "lockfile.h"
@@ -173,7 +174,7 @@ static int too_many_packs(void)
return 0;
 
prepare_packed_git();
-   for (cnt = 0, p = 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 83dcbc9773..0e5fde1d6b