Kirill Smelkov <kirr@xxxxxxxxxx> writes:
> Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
> are two codepaths in pack-objects: with & without using bitmap
> reachability index.
>
> However add_object_entry_from_bitmap(), despite its non-bitmapped
> counterpart add_object_entry(), in no way does check for whether --local
> or --honor-pack-keep or --incremental should be respected. In
> non-bitmapped codepath this is handled in want_object_in_pack(), but
> bitmapped codepath has simply no such checking at all.
>
> The bitmapped codepath however was allowing to pass in all those options
> and with bitmap indices still being used under such conditions -
> potentially giving wrong output (e.g. including objects from non-local or
> .keep'ed pack).
>
> We can easily fix this by noting the following: when an object comes to
> add_object_entry_from_bitmap() it can come for two reasons:
>
> 1. entries coming from main pack covered by bitmap index, and
> 2. object coming from, possibly alternate, loose or other packs.
>
> For "2" we always have pack not yet found by bitmap traversal code, and
> thus we can simply reuse non-bitmapped want_object_in_pack() to find in
> which pack an object lives and also for taking omitting decision.
>
> For "1" we always have pack already found by bitmap traversal code and we
> only need to check that pack for same criteria used in
> want_object_in_pack() for found_pack.
>
> Suggested-by: Junio C Hamano <gitster@xxxxxxxxx>
> Discussed-with: Jeff King <peff@xxxxxxxx>
> ---
I do not think I suggested much of this to deserve credit like this,
though, as I certainly haven't thought about the pros-and-cons
between adding the same "some object in pack may not want to be in
the output" logic to the bitmap side, or punting the bitmap codepath
when local/keep are involved.
> +/* Like want_object_in_pack() but for objects coming from-under bitmapped traversal */
> +static int want_object_in_pack_bitmap(const unsigned char *sha1,
> + struct packed_git **found_pack,
> + off_t *found_offset)
> +{
> + struct packed_git *p = *found_pack;
> +
> + /*
> + * There are two types of requests coming here:
> + * 1. entries coming from main pack covered by bitmap index, and
> + * 2. object coming from, possibly alternate, loose or other packs.
> + *
> + * For "1" we always have *found_pack != NULL passed here from
> + * traverse_bitmap_commit_list(). (*found_pack is bitmap_git.pack
> + * actually).
> + *
> + * For "2" we always have *found_pack == NULL passed here from
> + * traverse_bitmap_commit_list() - since this is the way bitmap
> + * traversal passes here "extended" bitmap entries.
> + */
> +
> + /* objects not covered by bitmap */
> + if (!p)
> + return want_object_in_pack(sha1, 0, found_pack, found_offset);
> + /* objects covered by bitmap - we only have to check p wrt local and .keep */
I am assuming that p != NULL only means "this object exists in THIS
pack", without saying anything about "this object may also exist in
other places", but "we only have to check" implies that "p != NULL"
means "this object exists *ONLY* in this pack and nowhere else".
Puzzled.
> + if (incremental)
> + return 0;
> + if (local && !p->pack_local)
> + return 0;
> + if (ignore_packed_keep && p->pack_local && p->pack_keep)
> + return 0;
> +
> + return 1;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html