Re: [PATCH 28/40] pack-objects: don't pack objects in external odbs

[Christian Couder]

On Thu, Jan 4, 2018 at 9:54 PM, Jeff Hostetler  wrote:
>
> On 1/3/2018 11:33 AM, Christian Couder wrote:
>>
>> Objects managed by an external ODB should not be put into
>> pack files. They should be transfered using other mechanism
>> that can be specific to the external odb.
>>
>> Signed-off-by: Christian Couder 
>> ---
>>   builtin/pack-objects.c | 4 
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index 6c71552cdf..4ed66c7677 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -28,6 +28,7 @@
>>   #include "argv-array.h"
>>   #include "mru.h"
>>   #include "packfile.h"
>> +#include "external-odb.h"
>> static const char *pack_usage[] = {
>> N_("git pack-objects --stdout [...] [<  | <
>> ]"),
>> @@ -1026,6 +1027,9 @@ static int want_object_in_pack(const struct
>> object_id *oid,
>> return want;
>> }
>>   + if (external_odb_has_object(oid->hash))
>> +   return 0;
>
> I worry about the performance of this in light of my comments
> earlier in the patch series about the expense of building the
> "have" sets.

Building the have set is done only if the cap_have capability is used,
(see my answer to your comments on patch 15/40).

> Since we've already checked for a loose object and we are about
> to walk thru the local packfiles, so if we don't find it in any
> of them, then we don't have it locally.  Only then do we need
> to worry about external odbs.
>
> If we don't have it locally, does the caller of this function
> have sufficient "promisor" infomation infer that the object
> should exist on the promisor remote?   Since you're going to
> omit it from the packfile anyway, you don't really need to know
> if the remote actually has it.

Yeah, I think it works in the same way as how the promisor code works,
but I haven't checked carefully, so I will take a look at that.

Thanks,
Christian.

Re: [PATCH 28/40] pack-objects: don't pack objects in external odbs

[Jeff Hostetler]

On 1/3/2018 11:33 AM, Christian Couder wrote:

Objects managed by an external ODB should not be put into
pack files. They should be transfered using other mechanism
that can be specific to the external odb.

Signed-off-by: Christian Couder 
---
  builtin/pack-objects.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6c71552cdf..4ed66c7677 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -28,6 +28,7 @@
  #include "argv-array.h"
  #include "mru.h"
  #include "packfile.h"
+#include "external-odb.h"
  
  static const char *pack_usage[] = {

N_("git pack-objects --stdout [...] [<  | < 
]"),
@@ -1026,6 +1027,9 @@ static int want_object_in_pack(const struct object_id 
*oid,
return want;
}
  
+	if (external_odb_has_object(oid->hash))

+   return 0;
+


I worry about the performance of this in light of my comments
earlier in the patch series about the expense of building the
"have" sets.

Since we've already checked for a loose object and we are about
to walk thru the local packfiles, so if we don't find it in any
of them, then we don't have it locally.  Only then do we need
to worry about external odbs.

If we don't have it locally, does the caller of this function
have sufficient "promisor" infomation infer that the object
should exist on the promisor remote?   Since you're going to
omit it from the packfile anyway, you don't really need to know
if the remote actually has it.



for (entry = packed_git_mru.head; entry; entry = entry->next) {
struct packed_git *p = entry->item;
off_t offset;



Jeff

[PATCH 28/40] pack-objects: don't pack objects in external odbs

[Christian Couder]

Objects managed by an external ODB should not be put into
pack files. They should be transfered using other mechanism
that can be specific to the external odb.

Signed-off-by: Christian Couder 
---
 builtin/pack-objects.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6c71552cdf..4ed66c7677 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -28,6 +28,7 @@
 #include "argv-array.h"
 #include "mru.h"
 #include "packfile.h"
+#include "external-odb.h"
 
 static const char *pack_usage[] = {
N_("git pack-objects --stdout [...] [<  | < 
]"),
@@ -1026,6 +1027,9 @@ static int want_object_in_pack(const struct object_id 
*oid,
return want;
}
 
+   if (external_odb_has_object(oid->hash))
+   return 0;
+
for (entry = packed_git_mru.head; entry; entry = entry->next) {
struct packed_git *p = entry->item;
off_t offset;
-- 
2.16.0.rc0.16.g82191dbc6c.dirty