{"thread":{"id":"64080","subject":"[PATCH 0/2] upload-pack: don't ACK non-commits repeatedly","startedAt":"2025-09-03T04:54:27Z","lastAt":"2025-09-08T16:51:26Z","messageCount":13,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"525391","messageId":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-0-4e019af4dddc@pks.im","threadId":"64080","inReplyTo":null,"subject":"[PATCH 0/2] upload-pack: don't ACK non-commits repeatedly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T04:54:09Z","receivedAt":"2025-09-03T04:54:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series addresses an issue with storing duplicate object IDs\nsent by the client in git-upload-pack(1). If the client sends repeated\n\"have\" lines for an object ID that doesn't refer to a commit, then we\nend up storing that object ID repeatedly in the `have_obj` array. This\nleads to sending out repeated \"ACK\"s for the same object.\n\nThe series applies on top of \"maint\" at c44beea485 (Git 2.51,\n2025-08-17).\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      t5530: modernize tests\n      upload-pack: don't ACK non-commits repeatedly in protocol v2\n\n t/t5530-upload-pack-error.sh | 68 +++++++++++++++++++++++++++-----------------\n upload-pack.c                | 19 ++++++-------\n 2 files changed, 51 insertions(+), 36 deletions(-)\n\n\n---\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nchange-id: 20250903-b4-pks-upload-pack-repeated-non-commit-acks-49b363e1d0e1\n\n"},{"id":"525392","messageId":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-1-4e019af4dddc@pks.im","threadId":"64080","inReplyTo":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-0-4e019af4dddc@pks.im","subject":"[PATCH 1/2] t5530: modernize tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T04:54:10Z","receivedAt":"2025-09-03T04:54:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor tests to follow modern best practices:\n\n  - Merge together tests that set up and verify a single use case.\n\n  - Drop empty newlines at the beginning and end of test bodies.\n\n  - Don't change directories in the main test body.\n\n  - Remove an unused `D` variable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t5530-upload-pack-error.sh | 33 +++++----------------------------\n 1 file changed, 5 insertions(+), 28 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 558eedf25a4..8e505786f1b 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -4,8 +4,6 @@ test_description='errors in upload-pack'\n \n . ./test-lib.sh\n \n-D=$(pwd)\n-\n corrupt_repo () {\n \tobject_sha1=$(git rev-parse \"$1\") &&\n \tob=$(expr \"$object_sha1\" : \"\\(..\\)\") &&\n@@ -21,11 +19,7 @@ test_expect_success 'setup and corrupt repository' '\n \ttest_tick &&\n \techo changed >file &&\n \tgit commit -a -m changed &&\n-\tcorrupt_repo HEAD:file\n-\n-'\n-\n-test_expect_success 'fsck fails' '\n+\tcorrupt_repo HEAD:file &&\n \ttest_must_fail git fsck\n '\n \n@@ -40,17 +34,12 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' '\n '\n \n test_expect_success 'corrupt repo differently' '\n-\n \tgit hash-object -w file &&\n-\tcorrupt_repo HEAD^^{tree}\n-\n-'\n-\n-test_expect_success 'fsck fails' '\n+\tcorrupt_repo HEAD^^{tree} &&\n \ttest_must_fail git fsck\n '\n-test_expect_success 'upload-pack fails due to error in rev-list' '\n \n+test_expect_success 'upload-pack fails due to error in rev-list' '\n \tprintf \"%04xwant %s\\n%04xshallow %s00000009done\\n0000\" \\\n \t\t$(($hexsz + 10)) $(git rev-parse HEAD) \\\n \t\t$(($hexsz + 12)) $(git rev-parse HEAD^) >input &&\n@@ -59,7 +48,6 @@ test_expect_success 'upload-pack fails due to error in rev-list' '\n '\n \n test_expect_success 'upload-pack fails due to bad want (no object)' '\n-\n \tprintf \"%04xwant %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t$(($hexsz + 29)) $(test_oid deadbeef) >input &&\n \ttest_must_fail git upload-pack . <input >output 2>output.err &&\n@@ -69,7 +57,6 @@ test_expect_success 'upload-pack fails due to bad want (no object)' '\n '\n \n test_expect_success 'upload-pack fails due to bad want (not tip)' '\n-\n \toid=$(echo an object we have | git hash-object -w --stdin) &&\n \tprintf \"%04xwant %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t$(($hexsz + 29)) \"$oid\" >input &&\n@@ -80,7 +67,6 @@ test_expect_success 'upload-pack fails due to bad want (not tip)' '\n '\n \n test_expect_success 'upload-pack fails due to error in pack-objects enumeration' '\n-\n \tprintf \"%04xwant %s\\n00000009done\\n0000\" \\\n \t\t$((hexsz + 10)) $(git rev-parse HEAD) >input &&\n \ttest_must_fail git upload-pack . <input >/dev/null 2>output.err &&\n@@ -105,18 +91,9 @@ test_expect_success 'upload-pack tolerates EOF just after stateless client wants\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'create empty repository' '\n-\n-\tmkdir foo &&\n-\tcd foo &&\n-\tgit init\n-\n-'\n-\n test_expect_success 'fetch fails' '\n-\n-\ttest_must_fail git fetch .. main\n-\n+\tgit init foo &&\n+\ttest_must_fail git -C foo fetch .. main\n '\n \n test_done\n\n-- \n2.51.0.384.g4c02a37b29.dirty\n\n"},{"id":"525393","messageId":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-2-4e019af4dddc@pks.im","threadId":"64080","inReplyTo":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-0-4e019af4dddc@pks.im","subject":"[PATCH 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T04:54:11Z","receivedAt":"2025-09-03T04:54:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When a client performs a fetch or clone they can optionally send \"have\"\nlines to tell the server which objects they already have available\nlocally. These object IDs are stored by the server in an object array so\nthat it can remember any objects it doesn't have to include in the pack\nsent to the client.\n\nWhile there isn't any reason to do so, clients are free to send the same\n\"have\" line repeatedly. git-upload-pack(1) already knows to handle this\nwell: every commit it has seen via a \"have\" line gets marked with the\n`THEY_HAVE` flag, and if such a commit is seen repeatedly we know to not\nprocess it another time. This also has the effect that we only store the\nobject ID once, only, in the `have_obj` array.\n\nThere is an edge case though: if the client sends an object ID that does\nnot refer to a commit we neither store nor check the `THEY_HAVE` flag.\nThis means that we repeatedly store the same object ID in our `have_obj`\narray, with two consequences:\n\n  - In protocol v2 we deduplicate ACKs for commits, but not for any\n    other objects as we send ACKs for every object ID in the `have_obj`\n    array.\n\n  - The `have_obj` array can grow in size indefinitely with both\n    protocols.\n\nThe potentially-more-serious issue is the second one, as we basically\nhave a way for an adversary to allocate arbitrarily large buffers now.\nUltimately, this doesn't seem to be all that serious though: on my\nmachine, the growth of that array is at around 4MB/s, and after roughly\nfive minutes I was only at 1GB RSS. So this is concerning, but only\nmildly so.\n\nFix this bug by storing the `THEY_HAVE` flag independent of the object\ntype so that we don't store duplicate object IDs in `have_obj` anymore.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t5530-upload-pack-error.sh | 39 +++++++++++++++++++++++++++++++++++++++\n upload-pack.c                | 19 +++++++++----------\n 2 files changed, 48 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 8e505786f1..d40292cfb7 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -96,4 +96,43 @@ test_expect_success 'fetch fails' '\n \ttest_must_fail git -C foo fetch .. main\n '\n \n+test_expect_success 'upload-pack ACKs repeated non-commit objects repeatedly (protocol v0)' '\n+\tcommit_id=$(git rev-parse HEAD) &&\n+\ttree_id=$(git rev-parse HEAD^{tree}) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\twant $commit_id\n+\t0000\n+\thave $tree_id\n+\thave $tree_id\n+\t0000\n+\tEOF\n+\tgit upload-pack --stateless-rpc . <request >actual &&\n+\tdepacketize <actual >actual.raw &&\n+\tgrep ^ACK actual.raw >actual.acks &&\n+\tcat >expect <<-EOF &&\n+\tACK $tree_id\n+\tACK $tree_id\n+\tEOF\n+\ttest_cmp expect actual.acks\n+'\n+\n+test_expect_success 'upload-pack ACKs repeated non-commit objects once only (protocol v2)' '\n+\tcommit_id=$(git rev-parse HEAD) &&\n+\ttree_id=$(git rev-parse HEAD^{tree}) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\twant $commit_id\n+\thave $tree_id\n+\thave $tree_id\n+\t0000\n+\tEOF\n+\tGIT_PROTOCOL=version=2 git upload-pack . <request >actual &&\n+\tdepacketize <actual >actual.raw &&\n+\tgrep ^ACK actual.raw >actual.acks &&\n+\techo \"ACK $tree_id\" >expect &&\n+\ttest_cmp expect actual.acks\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4f26f6afc7..fba3e339e2 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -476,20 +476,21 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \n static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)\n {\n-\tint we_knew_they_have = 0;\n \tstruct object *o = parse_object_with_flags(the_repository, oid,\n \t\t\t\t\t\t   PARSE_OBJECT_SKIP_HASH_CHECK |\n \t\t\t\t\t\t   PARSE_OBJECT_DISCARD_TREE);\n \n \tif (!o)\n \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n+\n+\tif (o->flags & THEY_HAVE)\n+\t\treturn 0;\n+\to->flags |= THEY_HAVE;\n+\n \tif (o->type == OBJ_COMMIT) {\n \t\tstruct commit_list *parents;\n \t\tstruct commit *commit = (struct commit *)o;\n-\t\tif (o->flags & THEY_HAVE)\n-\t\t\twe_knew_they_have = 1;\n-\t\telse\n-\t\t\to->flags |= THEY_HAVE;\n+\n \t\tif (!data->oldest_have || (commit->date < data->oldest_have))\n \t\t\tdata->oldest_have = commit->date;\n \t\tfor (parents = commit->parents;\n@@ -497,11 +498,9 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n \t\t     parents = parents->next)\n \t\t\tparents->item->object.flags |= THEY_HAVE;\n \t}\n-\tif (!we_knew_they_have) {\n-\t\tadd_object_array(o, NULL, &data->have_obj);\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n+\n+\tadd_object_array(o, NULL, &data->have_obj);\n+\treturn 1;\n }\n \n static int got_oid(struct upload_pack_data *data,\n\n-- \n2.51.0.384.g4c02a37b29.dirty\n\n"},{"id":"525478","messageId":"xmqqzfbac0yv.fsf@gitster.g","threadId":"64080","inReplyTo":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-1-4e019af4dddc@pks.im","subject":"Re: [PATCH 1/2] t5530: modernize tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T04:10:00Z","receivedAt":"2025-09-04T04:10:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> -D=$(pwd)\n> -\n> @@ -21,11 +19,7 @@ test_expect_success 'setup and corrupt repository' '\n>  \ttest_tick &&\n>  \techo changed >file &&\n>  \tgit commit -a -m changed &&\n> -\tcorrupt_repo HEAD:file\n> -\n> -'\n> -\n> -test_expect_success 'fsck fails' '\n> +\tcorrupt_repo HEAD:file &&\n>  \ttest_must_fail git fsck\n>  '\n>  \n> @@ -40,17 +34,12 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' '\n>  '\n>  \n>  test_expect_success 'corrupt repo differently' '\n> -\n>  \tgit hash-object -w file &&\n> -\tcorrupt_repo HEAD^^{tree}\n> -\n> -'\n> -\n> -test_expect_success 'fsck fails' '\n> +\tcorrupt_repo HEAD^^{tree} &&\n>  \ttest_must_fail git fsck\n>  '\n\nBoth changes make sense.\n\n> -test_expect_success 'create empty repository' '\n> -\n> -\tmkdir foo &&\n> -\tcd foo &&\n> -\tgit init\n> -\n> -'\n> -\n>  test_expect_success 'fetch fails' '\n> -\n> -\ttest_must_fail git fetch .. main\n> -\n> +\tgit init foo &&\n> +\ttest_must_fail git -C foo fetch .. main\n>  '\n\nNice.\n"},{"id":"525479","messageId":"xmqqtt1ic0ci.fsf@gitster.g","threadId":"64080","inReplyTo":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-2-4e019af4dddc@pks.im","subject":"Re: [PATCH 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T04:23:25Z","receivedAt":"2025-09-04T04:23:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> This means that we repeatedly store the same object ID in our `have_obj`\n> array, with two consequences:\n>\n>   - In protocol v2 we deduplicate ACKs for commits, but not for any\n>     other objects as we send ACKs for every object ID in the `have_obj`\n>     array.\n>\n>   - The `have_obj` array can grow in size indefinitely with both\n>     protocols.\n>\n> The potentially-more-serious issue is the second one, as we basically\n> have a way for an adversary to allocate arbitrarily large buffers now.\n> Ultimately, this doesn't seem to be all that serious though: on my\n> machine, the growth of that array is at around 4MB/s, and after roughly\n> five minutes I was only at 1GB RSS. So this is concerning, but only\n> mildly so.\n>\n> Fix this bug by storing the `THEY_HAVE` flag independent of the object\n> type so that we don't store duplicate object IDs in `have_obj` anymore.\n\nMakes sense.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 4f26f6afc7..fba3e339e2 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -476,20 +476,21 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n>  \n>  static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)\n>  {\n> -\tint we_knew_they_have = 0;\n>  \tstruct object *o = parse_object_with_flags(the_repository, oid,\n>  \t\t\t\t\t\t   PARSE_OBJECT_SKIP_HASH_CHECK |\n>  \t\t\t\t\t\t   PARSE_OBJECT_DISCARD_TREE);\n>  \n>  \tif (!o)\n>  \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n> +\n> +\tif (o->flags & THEY_HAVE)\n> +\t\treturn 0;\n> +\to->flags |= THEY_HAVE;\n> +\n>  \tif (o->type == OBJ_COMMIT) {\n>  \t\tstruct commit_list *parents;\n>  \t\tstruct commit *commit = (struct commit *)o;\n> -\t\tif (o->flags & THEY_HAVE)\n> -\t\t\twe_knew_they_have = 1;\n> -\t\telse\n> -\t\t\to->flags |= THEY_HAVE;\n> +\n\nOK, so regardless of the type, an object that we already know they\nhave do not need to be processed twice, which makes sense?\n\nFor commits, we used to ensure that even if we see a commit that we\nknow they have for the second time, instead of returning, we marked\nits immediate parents with THEY_HAVE bit.  We no longer do so.\n\nInstead, the new code only paints the parents of a commit that we\nhaven't painted with THEY_HAVE.  Otherwise we return without doing\nanything.\n\nI wonder if they are equivalent.  In the original code, when our\ncommit A has a parent B, where neither of them we haven't painted,\nwe paint A with THEY_HAVE, we paint all parents of A with THEY_HAVE,\nand put A in the have_obj array and return 1.  Then later when we\nfeed B to this function, we already know they have B, so we do not\nadd it to have_obj array, but otherwise we still paint B's parents\nwith THEY_HAVE bit.  In the new code with the same set-up, we paint\nA and B with THEY_HAVE and put A in the have_obj array and return 1.\nWhen the caller feeds B to this function, we pretty much do nothing\nand return, so B's parent will not be painted, will it?\n\nI must be misreading the patch, but I do not quite see how.\n\n>  \t\tif (!data->oldest_have || (commit->date < data->oldest_have))\n>  \t\t\tdata->oldest_have = commit->date;\n>  \t\tfor (parents = commit->parents;\n> @@ -497,11 +498,9 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n>  \t\t     parents = parents->next)\n>  \t\t\tparents->item->object.flags |= THEY_HAVE;\n>  \t}\n> -\tif (!we_knew_they_have) {\n> -\t\tadd_object_array(o, NULL, &data->have_obj);\n> -\t\treturn 1;\n> -\t}\n> -\treturn 0;\n\n> +\n> +\tadd_object_array(o, NULL, &data->have_obj);\n> +\treturn 1;\n>  }\n>  \n>  static int got_oid(struct upload_pack_data *data,\n"},{"id":"525494","messageId":"aLmJOdxUYiyHpiLA@pks.im","threadId":"64080","inReplyTo":"xmqqtt1ic0ci.fsf@gitster.g","subject":"Re: [PATCH 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-04T12:42:33Z","receivedAt":"2025-09-04T12:42:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Sep 03, 2025 at 09:23:25PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 4f26f6afc7..fba3e339e2 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -476,20 +476,21 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n> >  \n> >  static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)\n> >  {\n> > -\tint we_knew_they_have = 0;\n> >  \tstruct object *o = parse_object_with_flags(the_repository, oid,\n> >  \t\t\t\t\t\t   PARSE_OBJECT_SKIP_HASH_CHECK |\n> >  \t\t\t\t\t\t   PARSE_OBJECT_DISCARD_TREE);\n> >  \n> >  \tif (!o)\n> >  \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n> > +\n> > +\tif (o->flags & THEY_HAVE)\n> > +\t\treturn 0;\n> > +\to->flags |= THEY_HAVE;\n> > +\n> >  \tif (o->type == OBJ_COMMIT) {\n> >  \t\tstruct commit_list *parents;\n> >  \t\tstruct commit *commit = (struct commit *)o;\n> > -\t\tif (o->flags & THEY_HAVE)\n> > -\t\t\twe_knew_they_have = 1;\n> > -\t\telse\n> > -\t\t\to->flags |= THEY_HAVE;\n> > +\n> \n> OK, so regardless of the type, an object that we already know they\n> have do not need to be processed twice, which makes sense?\n> \n> For commits, we used to ensure that even if we see a commit that we\n> know they have for the second time, instead of returning, we marked\n> its immediate parents with THEY_HAVE bit.  We no longer do so.\n> \n> Instead, the new code only paints the parents of a commit that we\n> haven't painted with THEY_HAVE.  Otherwise we return without doing\n> anything.\n> \n> I wonder if they are equivalent.  In the original code, when our\n> commit A has a parent B, where neither of them we haven't painted,\n> we paint A with THEY_HAVE, we paint all parents of A with THEY_HAVE,\n> and put A in the have_obj array and return 1.  Then later when we\n> feed B to this function, we already know they have B, so we do not\n> add it to have_obj array, but otherwise we still paint B's parents\n> with THEY_HAVE bit.  In the new code with the same set-up, we paint\n> A and B with THEY_HAVE and put A in the have_obj array and return 1.\n> When the caller feeds B to this function, we pretty much do nothing\n> and return, so B's parent will not be painted, will it?\n> \n> I must be misreading the patch, but I do not quite see how.\n\nOh, I think you're indeed correct. I totally missed that we wouldn't\npaint the grandparents anymore with this change. It's quite surprising\nthat this didn't cause any issues whatsoever. But that might be due to\nhow clients typically negotiate HAVEs with the remote side: when the\nserver ACKs an object there is no reason for the client to go deeper, so\nthey stop advertising any of its parents.\n\nThere will be cases where this breaks though. So we should probably move\nthe return until after we have marked parents, like in the below patch.\n\nPatrick\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex fba3e339e2..9b9b149068 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -483,10 +483,6 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n \tif (!o)\n \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n \n-\tif (o->flags & THEY_HAVE)\n-\t\treturn 0;\n-\to->flags |= THEY_HAVE;\n-\n \tif (o->type == OBJ_COMMIT) {\n \t\tstruct commit_list *parents;\n \t\tstruct commit *commit = (struct commit *)o;\n@@ -499,6 +495,10 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n \t\t\tparents->item->object.flags |= THEY_HAVE;\n \t}\n \n+\tif (o->flags & THEY_HAVE)\n+\t\treturn 0;\n+\to->flags |= THEY_HAVE;\n+\n \tadd_object_array(o, NULL, &data->have_obj);\n \treturn 1;\n }\n"},{"id":"525547","messageId":"xmqqqzwm9nrt.fsf@gitster.g","threadId":"64080","inReplyTo":"aLmJOdxUYiyHpiLA@pks.im","subject":"Re: [PATCH 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T16:37:58Z","receivedAt":"2025-09-04T16:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> ... when the\n> server ACKs an object there is no reason for the client to go deeper, so\n> they stop advertising any of its parents.\n\nVery true.  I did point out the difference in behaviour, but I\ncouldn't actually figure out what goodness we are deriving in this\ncode by marking grandparents (and possibly their ancestors) as\nsomething they have.  Your change in behaviour could be seen as\nstopping us from making unnecessary operations ;-)\n\nIf the other side is mischievous, they are not limited to feed us a\ncommit and then its parent in the naturally expected order.  They\ncould feed us A and then skip B and give us C that is a child of B.\nPre-painting B when we got A from them, in order to prepare for\nseeing B (which we can return without doing anything) in the next\nround, would not help us all that much, if they give us C after\ngiving us A, as we haven't even heard of C yet at that point.\n\nBut in the normal case against sane clients that do not skip the\nprobes, marking immediate parents (like B) when processing A might\nbe helping?  I dunno.  I also somehow thought that even normal case\nwe have an option to skip the probes in order to converge faster,\nbut I am misremembering.\n\ncf. https://lore.kernel.org/git/?q=upload-pack%20fibonacci\n\n\n\n"},{"id":"525588","messageId":"aLp_JdWjpxF3sz17@pks.im","threadId":"64080","inReplyTo":"xmqqqzwm9nrt.fsf@gitster.g","subject":"Re: [PATCH 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-05T06:11:49Z","receivedAt":"2025-09-05T06:11:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 04, 2025 at 09:37:58AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > ... when the\n> > server ACKs an object there is no reason for the client to go deeper, so\n> > they stop advertising any of its parents.\n> \n> Very true.  I did point out the difference in behaviour, but I\n> couldn't actually figure out what goodness we are deriving in this\n> code by marking grandparents (and possibly their ancestors) as\n> something they have.  Your change in behaviour could be seen as\n> stopping us from making unnecessary operations ;-)\n> \n> If the other side is mischievous, they are not limited to feed us a\n> commit and then its parent in the naturally expected order.  They\n> could feed us A and then skip B and give us C that is a child of B.\n> Pre-painting B when we got A from them, in order to prepare for\n> seeing B (which we can return without doing anything) in the next\n> round, would not help us all that much, if they give us C after\n> giving us A, as we haven't even heard of C yet at that point.\n\nYeah, it _feels_ like the change should be fine overall. But honestly, I\nfeel more comfortable with keeping the status quo for now so that the\nchange is only doing what it's advertised to do, namely cull the memory\ngrowth.\n\nSo I'll apply the patch I had and send a v2, but we may think about the\nother angle as a #leftoverbit.\n\n> But in the normal case against sane clients that do not skip the\n> probes, marking immediate parents (like B) when processing A might\n> be helping?  I dunno.  I also somehow thought that even normal case\n> we have an option to skip the probes in order to converge faster,\n> but I am misremembering.\n> \n> cf. https://lore.kernel.org/git/?q=upload-pack%20fibonacci\n\nYou probably mean `fetch.negotiationAlgorithm=skipping`?\n\nPatrick\n"},{"id":"525589","messageId":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-0-d2e67f3cb94c@pks.im","threadId":"64080","inReplyTo":"20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-0-4e019af4dddc@pks.im","subject":"[PATCH v2 0/2] upload-pack: don't ACK non-commits repeatedly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-05T06:18:00Z","receivedAt":"2025-09-05T06:18:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series addresses an issue with storing duplicate object IDs\nsent by the client in git-upload-pack(1). If the client sends repeated\n\"have\" lines for an object ID that doesn't refer to a commit, then we\nend up storing that object ID repeatedly in the `have_obj` array. This\nleads to sending out repeated \"ACK\"s for the same object.\n\nThe series applies on top of \"maint\" at c44beea485 (Git 2.51,\n2025-08-17).\n\nChanges in v2:\n  - Change ordering so that we always mark parents of already-seen\n    commits as `THEY_HAVE`. The first version was _probably_ fine, but\n    I don't feel too comfortable with a \"probably\".\n  - Link to v1: https://lore.kernel.org/r/20250903-b4-pks-upload-pack-repeated-non-commit-acks-v1-0-4e019af4dddc@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n      t5530: modernize tests\n      upload-pack: don't ACK non-commits repeatedly in protocol v2\n\n t/t5530-upload-pack-error.sh | 68 +++++++++++++++++++++++++++-----------------\n upload-pack.c                | 19 ++++++-------\n 2 files changed, 51 insertions(+), 36 deletions(-)\n\nRange-diff versus v1:\n\n1:  a42d231813 = 1:  26ecbcf460 t5530: modernize tests\n2:  1544160961 ! 2:  11e32bf5e6 upload-pack: don't ACK non-commits repeatedly in protocol v2\n    @@ upload-pack.c: static void create_pack_file(struct upload_pack_data *pack_data,\n      \n      \tif (!o)\n      \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n    -+\n    -+\tif (o->flags & THEY_HAVE)\n    -+\t\treturn 0;\n    -+\to->flags |= THEY_HAVE;\n     +\n      \tif (o->type == OBJ_COMMIT) {\n      \t\tstruct commit_list *parents;\n    @@ upload-pack.c: static int do_got_oid(struct upload_pack_data *data, const struct\n     -\t}\n     -\treturn 0;\n     +\n    ++\tif (o->flags & THEY_HAVE)\n    ++\t\treturn 0;\n    ++\to->flags |= THEY_HAVE;\n    ++\n     +\tadd_object_array(o, NULL, &data->have_obj);\n     +\treturn 1;\n      }\n\n---\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nchange-id: 20250903-b4-pks-upload-pack-repeated-non-commit-acks-49b363e1d0e1\n\n"},{"id":"525590","messageId":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-1-d2e67f3cb94c@pks.im","threadId":"64080","inReplyTo":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-0-d2e67f3cb94c@pks.im","subject":"[PATCH v2 1/2] t5530: modernize tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-05T06:18:01Z","receivedAt":"2025-09-05T06:18:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor tests to follow modern best practices:\n\n  - Merge together tests that set up and verify a single use case.\n\n  - Drop empty newlines at the beginning and end of test bodies.\n\n  - Don't change directories in the main test body.\n\n  - Remove an unused `D` variable.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t5530-upload-pack-error.sh | 33 +++++----------------------------\n 1 file changed, 5 insertions(+), 28 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 558eedf25a4..8e505786f1b 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -4,8 +4,6 @@ test_description='errors in upload-pack'\n \n . ./test-lib.sh\n \n-D=$(pwd)\n-\n corrupt_repo () {\n \tobject_sha1=$(git rev-parse \"$1\") &&\n \tob=$(expr \"$object_sha1\" : \"\\(..\\)\") &&\n@@ -21,11 +19,7 @@ test_expect_success 'setup and corrupt repository' '\n \ttest_tick &&\n \techo changed >file &&\n \tgit commit -a -m changed &&\n-\tcorrupt_repo HEAD:file\n-\n-'\n-\n-test_expect_success 'fsck fails' '\n+\tcorrupt_repo HEAD:file &&\n \ttest_must_fail git fsck\n '\n \n@@ -40,17 +34,12 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' '\n '\n \n test_expect_success 'corrupt repo differently' '\n-\n \tgit hash-object -w file &&\n-\tcorrupt_repo HEAD^^{tree}\n-\n-'\n-\n-test_expect_success 'fsck fails' '\n+\tcorrupt_repo HEAD^^{tree} &&\n \ttest_must_fail git fsck\n '\n-test_expect_success 'upload-pack fails due to error in rev-list' '\n \n+test_expect_success 'upload-pack fails due to error in rev-list' '\n \tprintf \"%04xwant %s\\n%04xshallow %s00000009done\\n0000\" \\\n \t\t$(($hexsz + 10)) $(git rev-parse HEAD) \\\n \t\t$(($hexsz + 12)) $(git rev-parse HEAD^) >input &&\n@@ -59,7 +48,6 @@ test_expect_success 'upload-pack fails due to error in rev-list' '\n '\n \n test_expect_success 'upload-pack fails due to bad want (no object)' '\n-\n \tprintf \"%04xwant %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t$(($hexsz + 29)) $(test_oid deadbeef) >input &&\n \ttest_must_fail git upload-pack . <input >output 2>output.err &&\n@@ -69,7 +57,6 @@ test_expect_success 'upload-pack fails due to bad want (no object)' '\n '\n \n test_expect_success 'upload-pack fails due to bad want (not tip)' '\n-\n \toid=$(echo an object we have | git hash-object -w --stdin) &&\n \tprintf \"%04xwant %s multi_ack_detailed\\n00000009done\\n0000\" \\\n \t\t$(($hexsz + 29)) \"$oid\" >input &&\n@@ -80,7 +67,6 @@ test_expect_success 'upload-pack fails due to bad want (not tip)' '\n '\n \n test_expect_success 'upload-pack fails due to error in pack-objects enumeration' '\n-\n \tprintf \"%04xwant %s\\n00000009done\\n0000\" \\\n \t\t$((hexsz + 10)) $(git rev-parse HEAD) >input &&\n \ttest_must_fail git upload-pack . <input >/dev/null 2>output.err &&\n@@ -105,18 +91,9 @@ test_expect_success 'upload-pack tolerates EOF just after stateless client wants\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'create empty repository' '\n-\n-\tmkdir foo &&\n-\tcd foo &&\n-\tgit init\n-\n-'\n-\n test_expect_success 'fetch fails' '\n-\n-\ttest_must_fail git fetch .. main\n-\n+\tgit init foo &&\n+\ttest_must_fail git -C foo fetch .. main\n '\n \n test_done\n\n-- \n2.51.0.417.g1ba7204a04.dirty\n\n"},{"id":"525591","messageId":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-2-d2e67f3cb94c@pks.im","threadId":"64080","inReplyTo":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-0-d2e67f3cb94c@pks.im","subject":"[PATCH v2 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-05T06:18:02Z","receivedAt":"2025-09-05T06:18:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When a client performs a fetch or clone they can optionally send \"have\"\nlines to tell the server which objects they already have available\nlocally. These object IDs are stored by the server in an object array so\nthat it can remember any objects it doesn't have to include in the pack\nsent to the client.\n\nWhile there isn't any reason to do so, clients are free to send the same\n\"have\" line repeatedly. git-upload-pack(1) already knows to handle this\nwell: every commit it has seen via a \"have\" line gets marked with the\n`THEY_HAVE` flag, and if such a commit is seen repeatedly we know to not\nprocess it another time. This also has the effect that we only store the\nobject ID once, only, in the `have_obj` array.\n\nThere is an edge case though: if the client sends an object ID that does\nnot refer to a commit we neither store nor check the `THEY_HAVE` flag.\nThis means that we repeatedly store the same object ID in our `have_obj`\narray, with two consequences:\n\n  - In protocol v2 we deduplicate ACKs for commits, but not for any\n    other objects as we send ACKs for every object ID in the `have_obj`\n    array.\n\n  - The `have_obj` array can grow in size indefinitely with both\n    protocols.\n\nThe potentially-more-serious issue is the second one, as we basically\nhave a way for an adversary to allocate arbitrarily large buffers now.\nUltimately, this doesn't seem to be all that serious though: on my\nmachine, the growth of that array is at around 4MB/s, and after roughly\nfive minutes I was only at 1GB RSS. So this is concerning, but only\nmildly so.\n\nFix this bug by storing the `THEY_HAVE` flag independent of the object\ntype so that we don't store duplicate object IDs in `have_obj` anymore.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t5530-upload-pack-error.sh | 39 +++++++++++++++++++++++++++++++++++++++\n upload-pack.c                | 19 +++++++++----------\n 2 files changed, 48 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 8e505786f1..d40292cfb7 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -96,4 +96,43 @@ test_expect_success 'fetch fails' '\n \ttest_must_fail git -C foo fetch .. main\n '\n \n+test_expect_success 'upload-pack ACKs repeated non-commit objects repeatedly (protocol v0)' '\n+\tcommit_id=$(git rev-parse HEAD) &&\n+\ttree_id=$(git rev-parse HEAD^{tree}) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\twant $commit_id\n+\t0000\n+\thave $tree_id\n+\thave $tree_id\n+\t0000\n+\tEOF\n+\tgit upload-pack --stateless-rpc . <request >actual &&\n+\tdepacketize <actual >actual.raw &&\n+\tgrep ^ACK actual.raw >actual.acks &&\n+\tcat >expect <<-EOF &&\n+\tACK $tree_id\n+\tACK $tree_id\n+\tEOF\n+\ttest_cmp expect actual.acks\n+'\n+\n+test_expect_success 'upload-pack ACKs repeated non-commit objects once only (protocol v2)' '\n+\tcommit_id=$(git rev-parse HEAD) &&\n+\ttree_id=$(git rev-parse HEAD^{tree}) &&\n+\ttest-tool pkt-line pack >request <<-EOF &&\n+\tcommand=fetch\n+\tobject-format=$(test_oid algo)\n+\t0001\n+\twant $commit_id\n+\thave $tree_id\n+\thave $tree_id\n+\t0000\n+\tEOF\n+\tGIT_PROTOCOL=version=2 git upload-pack . <request >actual &&\n+\tdepacketize <actual >actual.raw &&\n+\tgrep ^ACK actual.raw >actual.acks &&\n+\techo \"ACK $tree_id\" >expect &&\n+\ttest_cmp expect actual.acks\n+'\n+\n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4f26f6afc7..9b9b149068 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -476,20 +476,17 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \n static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid)\n {\n-\tint we_knew_they_have = 0;\n \tstruct object *o = parse_object_with_flags(the_repository, oid,\n \t\t\t\t\t\t   PARSE_OBJECT_SKIP_HASH_CHECK |\n \t\t\t\t\t\t   PARSE_OBJECT_DISCARD_TREE);\n \n \tif (!o)\n \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n+\n \tif (o->type == OBJ_COMMIT) {\n \t\tstruct commit_list *parents;\n \t\tstruct commit *commit = (struct commit *)o;\n-\t\tif (o->flags & THEY_HAVE)\n-\t\t\twe_knew_they_have = 1;\n-\t\telse\n-\t\t\to->flags |= THEY_HAVE;\n+\n \t\tif (!data->oldest_have || (commit->date < data->oldest_have))\n \t\t\tdata->oldest_have = commit->date;\n \t\tfor (parents = commit->parents;\n@@ -497,11 +494,13 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n \t\t     parents = parents->next)\n \t\t\tparents->item->object.flags |= THEY_HAVE;\n \t}\n-\tif (!we_knew_they_have) {\n-\t\tadd_object_array(o, NULL, &data->have_obj);\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n+\n+\tif (o->flags & THEY_HAVE)\n+\t\treturn 0;\n+\to->flags |= THEY_HAVE;\n+\n+\tadd_object_array(o, NULL, &data->have_obj);\n+\treturn 1;\n }\n \n static int got_oid(struct upload_pack_data *data,\n\n-- \n2.51.0.417.g1ba7204a04.dirty\n\n"},{"id":"525678","messageId":"xmqq4itg37f6.fsf@gitster.g","threadId":"64080","inReplyTo":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-0-d2e67f3cb94c@pks.im","subject":"Re: [PATCH v2 0/2] upload-pack: don't ACK non-commits repeatedly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-05T21:39:57Z","receivedAt":"2025-09-05T21:40:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> this patch series addresses an issue with storing duplicate object IDs\n> sent by the client in git-upload-pack(1). If the client sends repeated\n> \"have\" lines for an object ID that doesn't refer to a commit, then we\n> end up storing that object ID repeatedly in the `have_obj` array. This\n> leads to sending out repeated \"ACK\"s for the same object.\n>\n> The series applies on top of \"maint\" at c44beea485 (Git 2.51,\n> 2025-08-17).\n>\n> Changes in v2:\n>   - Change ordering so that we always mark parents of already-seen\n>     commits as `THEY_HAVE`. The first version was _probably_ fine, but\n>     I don't feel too comfortable with a \"probably\".\n> ...\n> 2:  1544160961 ! 2:  11e32bf5e6 upload-pack: don't ACK non-commits repeatedly in protocol v2\n>     @@ upload-pack.c: static void create_pack_file(struct upload_pack_data *pack_data,\n>       \n>       \tif (!o)\n>       \t\tdie(\"oops (%s)\", oid_to_hex(oid));\n>     -+\n>     -+\tif (o->flags & THEY_HAVE)\n>     -+\t\treturn 0;\n>     -+\to->flags |= THEY_HAVE;\n>      +\n>       \tif (o->type == OBJ_COMMIT) {\n>       \t\tstruct commit_list *parents;\n>     @@ upload-pack.c: static int do_got_oid(struct upload_pack_data *data, const struct\n>      -\t}\n>      -\treturn 0;\n>      +\n>     ++\tif (o->flags & THEY_HAVE)\n>     ++\t\treturn 0;\n>     ++\to->flags |= THEY_HAVE;\n>     ++\n>      +\tadd_object_array(o, NULL, &data->have_obj);\n>      +\treturn 1;\n>       }\n\nOK.  So we used to do the \"if the object were marked already, do not\nbother adding it to the have_obj array again\" only for commits, but\nnow we do not special case commits for that part of the logic.\nEverybody is protected against getting added twice.\n\nWe still do special case commits by marking their direct parents\n(but we do not add them to the have_obj array) as before, because we\nwant to play conservatively.\n\nWhich makes sense.\n\nThanks, will queue.\n"},{"id":"525862","messageId":"20250908165125.GA1324063@coredump.intra.peff.net","threadId":"64080","inReplyTo":"20250905-b4-pks-upload-pack-repeated-non-commit-acks-v2-2-d2e67f3cb94c@pks.im","subject":"Re: [PATCH v2 2/2] upload-pack: don't ACK non-commits repeatedly in protocol v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-08T16:51:25Z","receivedAt":"2025-09-08T16:51:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 05, 2025 at 08:18:02AM +0200, Patrick Steinhardt wrote:\n\n>  \tif (o->type == OBJ_COMMIT) {\n>  \t\tstruct commit_list *parents;\n>  \t\tstruct commit *commit = (struct commit *)o;\n> -\t\tif (o->flags & THEY_HAVE)\n> -\t\t\twe_knew_they_have = 1;\n> -\t\telse\n> -\t\t\to->flags |= THEY_HAVE;\n> +\n>  \t\tif (!data->oldest_have || (commit->date < data->oldest_have))\n>  \t\t\tdata->oldest_have = commit->date;\n>  \t\tfor (parents = commit->parents;\n> @@ -497,11 +494,13 @@ static int do_got_oid(struct upload_pack_data *data, const struct object_id *oid\n>  \t\t     parents = parents->next)\n>  \t\t\tparents->item->object.flags |= THEY_HAVE;\n>  \t}\n> -\tif (!we_knew_they_have) {\n> -\t\tadd_object_array(o, NULL, &data->have_obj);\n> -\t\treturn 1;\n> -\t}\n> -\treturn 0;\n> +\n> +\tif (o->flags & THEY_HAVE)\n> +\t\treturn 0;\n> +\to->flags |= THEY_HAVE;\n> +\n> +\tadd_object_array(o, NULL, &data->have_obj);\n> +\treturn 1;\n>  }\n\nJust looking at the old code and its \"we_knew_they_have\" variable, I\nwondered why we didn't need it here anymore. In the old code, we set our\n(o->flags) THEY_HAVE flag up front, and then do some more processing on\nthe parents, and then decide on our return value. So we need to remember\nthe state before we set our flag.\n\nIn the new code, we do all of the parent processing and then return\nimmediately. So we can just do so by checking the flag without an extra\nvariable.\n\nAnd we know that is correct to check the flag after the parent\nprocessing because we should not be setting THEY_HAVE on o->flags as\npart of that. It would imply that a commit is its own parent.\n\nSo I think this looks fine, but I wanted to read through carefully (and\nout loud) to make sure there wasn't a subtle gotcha.\n\n-Peff\n"}]}