Re: Shallow clone

[Junio C Hamano <junkio@cox.net>]

Sergey Vlasov <vsu@altlinux.ru> writes:

> This is due to optimization in builtin-pack-objects.c:try_delta():
>
> 	/*
> 	 * We do not bother to try a delta that we discarded
> 	 * on an earlier try, but only when reusing delta data.
> 	 */
> 	if (!no_reuse_delta && trg_entry->in_pack &&
> 	    trg_entry->in_pack == src_entry->in_pack)
> 		return 0;
>
> After removing this part the shallow pack after clone is 2.6M, as it
> should be.
>
> The problem with this optimization is that it is only valid if we are
> repacking either the same set of objects as we did earlier, or its
> superset.  But if we are packing a subset of objects, there will be some
> objects in that subset which were delta-compressed in the original pack,
> but base objects for that deltas are not included in our subset -
> therefore we will be unable to reuse existing deltas, and with that
> optimization we will never try to use delta compression for these
> objects.
> ...
> So any partial fetch (shallow or not) from a mostly packed repository
> currently results in a suboptimal pack.

That is correct.  How about something like this?

I think the determination of "repacking_superset" may need to be
tweaked because existing packs may have overlaps, and the patch
counts them once per pack.


diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 69e5dd3..fb25124 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -64,6 +64,7 @@ struct object_entry {
 static unsigned char object_list_sha1[20];
 static int non_empty;
 static int no_reuse_delta;
+static int repacking_superset;
 static int local;
 static int incremental;
 static int allow_ofs_delta;
@@ -1172,10 +1173,13 @@ static int try_delta(struct unpacked *tr
 		return -1;
 
 	/*
-	 * We do not bother to try a delta that we discarded
-	 * on an earlier try, but only when reusing delta data.
+	 * When we are packing the superset of objects we have already
+	 * packed, we do not bother to try a delta that we discarded
+	 * on an earlier try.  This heuristic of course should not
+	 * kick in when we are not reusing delta, or we know we are
+	 * sending a subset of objects from a repository.
 	 */
-	if (!no_reuse_delta && trg_entry->in_pack &&
+	if (!no_reuse_delta && repacking_superset && trg_entry->in_pack &&
 	    trg_entry->in_pack == src_entry->in_pack)
 		return 0;
 
@@ -1493,6 +1497,16 @@ static void get_object_list(int ac, cons
 	traverse_commit_list(&revs, show_commit, show_object);
 }
 
+static int count_packed_objects(void)
+{
+	struct packed_git *p;
+	int cnt = 0;
+
+	for (p = packed_git; p; p = p->next)
+		cnt += num_packed_objects(p);
+	return cnt;
+}
+
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	SHA_CTX ctx;
@@ -1631,6 +1645,8 @@ int cmd_pack_objects(int argc, const cha
 	if (non_empty && !nr_result)
 		return 0;
 
+	repacking_superset = count_packed_objects() < nr_result;
+
 	SHA1_Init(&ctx);
 	list = sorted_by_sha;
 	for (i = 0; i < nr_result; i++) {