Re: [PATCH v2 0/2] upload-pack: don't ACK non-commits repeatedly

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this patch series addresses an issue with storing duplicate object IDs
> sent by the client in git-upload-pack(1). If the client sends repeated
> "have" lines for an object ID that doesn't refer to a commit, then we
> end up storing that object ID repeatedly in the `have_obj` array. This
> leads to sending out repeated "ACK"s for the same object.
>
> The series applies on top of "maint" at c44beea485 (Git 2.51,
> 2025-08-17).
>
> Changes in v2:
>   - Change ordering so that we always mark parents of already-seen
>     commits as `THEY_HAVE`. The first version was _probably_ fine, but
>     I don't feel too comfortable with a "probably".
> ...
> 2:  1544160961 ! 2:  11e32bf5e6 upload-pack: don't ACK non-commits repeatedly in protocol v2
>     @@ upload-pack.c: static void create_pack_file(struct upload_pack_data *pack_data,
>       
>       	if (!o)
>       		die("oops (%s)", oid_to_hex(oid));
>     -+
>     -+	if (o->flags & THEY_HAVE)
>     -+		return 0;
>     -+	o->flags |= THEY_HAVE;
>      +
>       	if (o->type == OBJ_COMMIT) {
>       		struct commit_list *parents;
>     @@ upload-pack.c: static int do_got_oid(struct upload_pack_data *data, const struct
>      -	}
>      -	return 0;
>      +
>     ++	if (o->flags & THEY_HAVE)
>     ++		return 0;
>     ++	o->flags |= THEY_HAVE;
>     ++
>      +	add_object_array(o, NULL, &data->have_obj);
>      +	return 1;
>       }

OK.  So we used to do the "if the object were marked already, do not
bother adding it to the have_obj array again" only for commits, but
now we do not special case commits for that part of the logic.
Everybody is protected against getting added twice.

We still do special case commits by marking their direct parents
(but we do not add them to the have_obj array) as before, because we
want to play conservatively.

Which makes sense.

Thanks, will queue.