Junio C Hamano <gitster@pobox.com> wrote:
quoted text
> This adds "--repack-unpacked" option to pack-objects to help
> combining small packs into one, without losing unreferenced
> objects that are in the packs.  When this option is given in
> addition to the above command line, we also make sure all the
> objects in the named existing packs are included in the result.

What about calling it "--keep-unreachable" instead?
 
quoted text
>  static void show_object(struct object_array_entry *p)
>  {
> +	struct object *o = lookup_unknown_object(p->item->sha1);

Can't you just say "o = p->item" here?

quoted text
> +struct in_pack_object {
> +	off_t offset;
> +	const unsigned char *sha1;
> +};

I can't help but think it would be better to store a "struct object*"
here instead of the "const unsigned char *sha1".  Both are of type
pointer but we only need the struct object* when are working with
the in_pack_object elements.

quoted text
> +static void mark_in_pack_object(const unsigned char *sha1, struct packed_git *p, struct in_pack *in_pack)
> +{
> +	in_pack->array[in_pack->nr].offset = find_pack_entry_one(sha1, p);
> +	in_pack->array[in_pack->nr].sha1 = sha1;
> +	in_pack->nr++;
> +}

Per above just pass the "struct object*" into this function and
store that into the array, instead of the sha1.  Of course you now
need to use "o->sha1" to perform the find_pack_entry_one().

quoted text
> +/*
> + * Compare the objects in the offset order, in order to emulate the
> + * "git-rev-list --objects" output that produced the pack originally.
> + */
> +static int ofscmp(const void *a_, const void *b_)
> +{
> +	struct in_pack_object *a = (struct in_pack_object *)a_;
> +	struct in_pack_object *b = (struct in_pack_object *)b_;
> +
> +	if (a->offset < b->offset)
> +		return -1;
> +	else if (a->offset > b->offset)
> +		return 1;
> +	else
> +		return hashcmp(a->sha1, b->sha1);
> +}

Really?  It is not possible for two objects to be placed at the
same offset within the same packfile and yet have two different
SHA-1 values.  The final else condition above is just "return 0".

quoted text
> +static void add_objects_in_unpacked_packs(struct rev_info *revs)
...
quoted text
> +		qsort(in_pack.array, in_pack.nr, sizeof(in_pack.array[0]),
> +		      ofscmp);
> +		for (i = 0; i < in_pack.nr; i++) {
> +			sha1 = in_pack.array[i].sha1;
> +			o = lookup_unknown_object(sha1);
> +			add_object_entry(sha1, o->type, "", 0);

If you save the "struct object*" into the in_pack_object (instead
of the SHA-1) as I described above you can avoid this second
lookup_unknown_object() call when we decide that we do need to pack
the object.

Otherwise I think it looks quite sane.

-- 
Shawn.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html