From: Jeff King <peff@peff.net>
Date: Mon, 08 Sep 2025 16:51:25 GMT
Subject: Re: [PATCH v2 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2
Message-ID: <20250908165125.GA1324063@coredump.intra.peff.net>
In-Reply-To: <20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-2-d2e67f3cb94c@pks.im>

On Fri, Sep 05, 2025 at 08:18:02AM +0200, Patrick Steinhardt wrote:

>  	if (o->type == OBJ_COMMIT) {
>  		struct commit_list *parents;
>  		struct commit *commit = (struct commit *)o;
> -		if (o->flags & THEY_HAVE)
> -			we_knew_they_have = 1;
> -		else
> -			o->flags |= THEY_HAVE;
> +
>  		if (!data->oldest_have || (commit->date < data->oldest_have))
>  			data->oldest_have = commit->date;
>  		for (parents = commit->parents;
> @@ -497,11 +494,13 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid
>  		     parents = parents->next)
>  			parents->item->object.flags |= THEY_HAVE;
>  	}
> -	if (!we_knew_they_have) {
> -		add_object_array(o, NULL, &data->have_obj);
> -		return 1;
> -	}
> -	return 0;
> +
> +	if (o->flags & THEY_HAVE)
> +		return 0;
> +	o->flags |= THEY_HAVE;
> +
> +	add_object_array(o, NULL, &data->have_obj);
> +	return 1;
>  }

Just looking at the old code and its "we_knew_they_have" variable, I
wondered why we didn't need it here anymore. In the old code, we set our
(o->flags) THEY_HAVE flag up front, and then do some more processing on
the parents, and then decide on our return value. So we need to remember
the state before we set our flag.

In the new code, we do all of the parent processing and then return
immediately. So we can just do so by checking the flag without an extra
variable.

And we know that is correct to check the flag after the parent
processing because we should not be setting THEY_HAVE on o->flags as
part of that. It would imply that a commit is its own parent.

So I think this looks fine, but I wanted to read through carefully (and
out loud) to make sure there wasn't a subtle gotcha.

-Peff