Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]