Re: [PATCH v2 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2
- From
Jeff King <peff@peff.net>
- Date
- Sep 8, 2025, 16:51 UTC
- 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:
Show 28 quoted lines
> 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