Web lists-archives.com

Re: [PATCH/RFC v3 02/12] pack-objects: turn type and in_pack_type to bitfields




Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:

> @@ -1570,7 +1576,7 @@ static void drop_reused_delta(struct object_entry *entry)
>  	entry->depth = 0;
>  
>  	oi.sizep = &entry->size;
> -	oi.typep = &entry->type;
> +	oi.typep = &type;
>  	if (packed_object_info(entry->in_pack, entry->in_pack_offset, &oi) < 0) {
>  		/*
>  		 * We failed to get the info from this pack for some reason;
> @@ -1580,6 +1586,10 @@ static void drop_reused_delta(struct object_entry *entry)
>  		 */
>  		entry->type = sha1_object_info(entry->idx.oid.hash,
>  					       &entry->size);

The comment immediately before this pre-context reads as such:

		/*
		 * We failed to get the info from this pack for some reason;
		 * fall back to sha1_object_info, which may find another copy.
		 * And if that fails, the error will be recorded in entry->type
		 * and dealt with in prepare_pack().
		 */

The rest of the code relies on the ability of entry->type to record
the error by storing an invalid (negative) type; otherwise, it cannot
detect an error where (1) the entry in _this_ pack was corrupt, and
(2) we wished to find another copy of the object elsewhere (which
would overwrite the negative entry->type we assign here), but we
didn't find any.

How should we propagate the error we found here down the control
flow in this new code?

> +	} else {
> +		if (type < 0)
> +			die("BUG: invalid type %d", type);
> +		entry->type = type;

The BUG() on this side is sensible, as packed_object_info()
shouldn't report success when it stored negative result in *oi.typep
anyway.

>  	unsigned char in_pack_header_size;
> +	unsigned type:TYPE_BITS;
> +	unsigned in_pack_type:TYPE_BITS; /* could be delta */