Re: [PATCH 01/13] pack-objects: extract should_attempt_deltas()

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 10, 2025 at 01:50:43AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> This will be helpful in a future change, which will reuse this logic.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  builtin/pack-objects.c | 53 +++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 58a9b161262..1d0992a8dac 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3196,6 +3196,33 @@ static int add_ref_tag(const char *tag UNUSED, const char *referent UNUSED, cons
>  	return 0;
>  }
>
> +static int should_attempt_deltas(struct object_entry *entry)
> +{
> +	if (DELTA(entry))
> +		return 0;
> +
> +	if (!entry->type_valid ||
> +	    oe_size_less_than(&to_pack, entry, 50))
> +		return 0;
> +
> +	if (entry->no_try_delta)
> +		return 0;
> +
> +	if (!entry->preferred_base) {
> +		if (oe_type(entry) < 0)
> +			die(_("unable to get type of object %s"),
> +				oid_to_hex(&entry->idx.oid));
> +	} else if (oe_type(entry) < 0) {
> +		/*
> +		 * This object is not found, but we
> +		 * don't have to include it anyway.
> +		 */
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static void prepare_pack(int window, int depth)
>  {
>  	struct object_entry **delta_list;
> @@ -3226,33 +3253,11 @@ static void prepare_pack(int window, int depth)
>  	for (i = 0; i < to_pack.nr_objects; i++) {
>  		struct object_entry *entry = to_pack.objects + i;
>
> -		if (DELTA(entry))
> -			/* This happens if we decided to reuse existing
> -			 * delta from a pack.  "reuse_delta &&" is implied.
> -			 */

It looks like this comment went away when this part of prepare_pack()
was extracted into should_attempt_deltas().

> -			continue;
> -
> -		if (!entry->type_valid ||
> -		    oe_size_less_than(&to_pack, entry, 50))
> +		if (!should_attempt_deltas(entry))
>  			continue;
>
> -		if (entry->no_try_delta)
> -			continue;
> -
> -		if (!entry->preferred_base) {
> +		if (!entry->preferred_base)
>  			nr_deltas++;

Makes sense; should_attempt_deltas() doesn't itself change nr_deltas, so
we want to do it ourselves here. Looking good!

Thanks,
Taylor