{"thread":{"id":"61530","subject":"[PATCH] push: don't fetch commit object when checking existence","startedAt":"2024-05-22T14:10:48Z","lastAt":"2024-05-23T08:58:24Z","messageCount":8,"participants":["Tom Hughes","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495300","messageId":"20240522133621.1308393-1-tom@compton.nu","threadId":"61530","inReplyTo":null,"subject":"[PATCH] push: don't fetch commit object when checking existence","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-22T13:36:21Z","receivedAt":"2024-05-22T14:10:48Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"If we're checking to see whether to tell the user to do a fetch\nbefore pushing there's no need for us to actually fetch the object\nfrom the remote if the clone is partial.\n\nBecause the promisor doesn't do negotiation actually trying to do\nthe fetch of the new head can be very expensive as it will try and\ninclude history that we already have and it just results in rejecting\nthe push with a different message, and in behavior that is different\nto a clone that is not partial.\n\nSigned-off-by: Tom Hughes <tom@compton.nu>\n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 2b650b813b..20395bbbd0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n-\t\t\telse if (!repo_has_object_file(the_repository, &ref->old_oid))\n+\t\t\telse if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\t\treject_reason = REF_STATUS_REJECT_FETCH_FIRST;\n \t\t\telse if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||\n \t\t\t\t !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))\n-- \n2.45.1\n\n"},{"id":"495320","messageId":"xmqq8r014pyn.fsf@gitster.g","threadId":"61530","inReplyTo":"20240522133621.1308393-1-tom@compton.nu","subject":"Re: [PATCH] push: don't fetch commit object when checking existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T19:16:32Z","receivedAt":"2024-05-22T19:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Hughes <tom@compton.nu> writes:\n\n> If we're checking to see whether to tell the user to do a fetch\n> before pushing there's no need for us to actually fetch the object\n> from the remote if the clone is partial.\n>\n> Because the promisor doesn't do negotiation actually trying to do\n> the fetch of the new head can be very expensive as it will try and\n> include history that we already have and it just results in rejecting\n> the push with a different message, and in behavior that is different\n> to a clone that is not partial.\n\nInteresting.  Is this something that is easily testable, perhaps by\npreparing a partial clone and try to push from there and checking\nthe non-existence of the object after seeing that push failed?\n\nThanks.\n\n> Signed-off-by: Tom Hughes <tom@compton.nu>\n> ---\n>  remote.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 2b650b813b..20395bbbd0 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>  \t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n>  \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n>  \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n> -\t\t\telse if (!repo_has_object_file(the_repository, &ref->old_oid))\n> +\t\t\telse if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n>  \t\t\t\treject_reason = REF_STATUS_REJECT_FETCH_FIRST;\n>  \t\t\telse if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||\n>  \t\t\t\t !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))\n"},{"id":"495323","messageId":"20240522201559.1677959-1-tom@compton.nu","threadId":"61530","inReplyTo":"xmqq8r014pyn.fsf@gitster.g","subject":"[PATCH v2] push: don't fetch commit object when checking existence","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-22T20:15:40Z","receivedAt":"2024-05-22T20:16:30Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"If we're checking to see whether to tell the user to do a fetch\nbefore pushing there's no need for us to actually fetch the object\nfrom the remote if the clone is partial.\n\nBecause the promisor doesn't do negotiation actually trying to do\nthe fetch of the new head can be very expensive as it will try and\ninclude history that we already have and it just results in rejecting\nthe push with a different message, and in behavior that is different\nto a clone that is not partial.\n\nSigned-off-by: Tom Hughes <tom@compton.nu>\n---\n remote.c                 |  2 +-\n t/t0410-partial-clone.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 2b650b813b..20395bbbd0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n-\t\t\telse if (!repo_has_object_file(the_repository, &ref->old_oid))\n+\t\t\telse if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\t\treject_reason = REF_STATUS_REJECT_FETCH_FIRST;\n \t\t\telse if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||\n \t\t\t\t !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 88a66f0904..7797391c03 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -689,6 +689,25 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '\n \t! grep \"[?]$FILE_HASH\" out\n '\n \n+test_expect_success 'push should not fetch new commit objects' '\n+\trm -rf server client &&\n+\ttest_create_repo server &&\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\ttest_commit -C server server1 &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\ttest_commit -C client client1 &&\n+\n+\ttest_commit -C server server2 &&\n+\tCOMMIT=$(git -C server rev-parse server2) &&\n+\n+\ttest_must_fail git -C client push 2>err &&\n+\tgrep \"fetch first\" err &&\n+\tgit -C client rev-list --objects --missing=print \"$COMMIT\" >objects &&\n+\tgrep \"^[?]$COMMIT\" objects\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.45.1\n\n"},{"id":"495324","messageId":"a546ca21-22ab-4a0d-9afa-e5c91830a5c2@compton.nu","threadId":"61530","inReplyTo":"xmqq8r014pyn.fsf@gitster.g","subject":"Re: [PATCH] push: don't fetch commit object when checking existence","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-22T20:18:00Z","receivedAt":"2024-05-22T20:18:07Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"On 22/05/2024 20:16, Junio C Hamano wrote:\n> Tom Hughes <tom@compton.nu> writes:\n> \n>> If we're checking to see whether to tell the user to do a fetch\n>> before pushing there's no need for us to actually fetch the object\n>> from the remote if the clone is partial.\n>>\n>> Because the promisor doesn't do negotiation actually trying to do\n>> the fetch of the new head can be very expensive as it will try and\n>> include history that we already have and it just results in rejecting\n>> the push with a different message, and in behavior that is different\n>> to a clone that is not partial.\n> \n> Interesting.  Is this something that is easily testable, perhaps by\n> preparing a partial clone and try to push from there and checking\n> the non-existence of the object after seeing that push failed?\n\nSure. I think I've managed to figure out a test and have sent\na second version of the patch with it added.\n\nTom\n\n-- \nTom Hughes (tom@compton.nu)\nhttp://compton.nu/\n\n"},{"id":"495328","messageId":"xmqqed9t36sn.fsf@gitster.g","threadId":"61530","inReplyTo":"20240522201559.1677959-1-tom@compton.nu","subject":"Re: [PATCH v2] push: don't fetch commit object when checking existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T20:55:52Z","receivedAt":"2024-05-22T20:56:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Hughes <tom@compton.nu> writes:\n\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 88a66f0904..7797391c03 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -689,6 +689,25 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '\n>  \t! grep \"[?]$FILE_HASH\" out\n>  '\n>  \n> +test_expect_success 'push should not fetch new commit objects' '\n> +\trm -rf server client &&\n> +\ttest_create_repo server &&\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\ttest_commit -C server server1 &&\n\nOK, we create the source that allows a partial clone.\n\n> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n> +\ttest_commit -C client client1 &&\n\nAnd make a clone out of it, without blobs.\n\n> +\ttest_commit -C server server2 &&\n> +\tCOMMIT=$(git -C server rev-parse server2) &&\n\nThen we create a new commit that the client does not yet have.\n\n> +\ttest_must_fail git -C client push 2>err &&\n\nWe try to overwrite it.  We expect it to fail with \"not a fast forward\".\n\n> +\tgrep \"fetch first\" err &&\n\nMay want to use \"test_grep\" but this script does not use it, so\nbeing consistent with the surrounding tests is good.\n\n> +\tgit -C client rev-list --objects --missing=print \"$COMMIT\" >objects &&\n> +\tgrep \"^[?]$COMMIT\" objects\n> +'\n\nOK.\n\n>  . \"$TEST_DIRECTORY\"/lib-httpd.sh\n>  start_httpd\n\nLooking good.  Thanks, will queue.\n"},{"id":"495335","messageId":"8f2ebf1b-050f-476a-92d4-dfb06ad04f8d@compton.nu","threadId":"61530","inReplyTo":"xmqqed9t36sn.fsf@gitster.g","subject":"Re: [PATCH v2] push: don't fetch commit object when checking existence","fromName":"Tom Hughes","fromEmail":"tom@compton.nu","sentAt":"2024-05-22T21:46:16Z","receivedAt":"2024-05-22T21:46:25Z","isPatch":true,"sender":{"key":"tom@compton.nu","avatar":"https://avatars.githubusercontent.com/u/147741?v=4"},"body":"On 22/05/2024 21:55, Junio C Hamano wrote:\n> Tom Hughes <tom@compton.nu> writes:\n>\n>> +test_expect_success 'push should not fetch new commit objects' '\n>> +\trm -rf server client &&\n>> +\ttest_create_repo server &&\n>> +\ttest_config -C server uploadpack.allowfilter 1 &&\n>> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n>> +\ttest_commit -C server server1 &&\n> \n> OK, we create the source that allows a partial clone.\n> \n>> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n>> +\ttest_commit -C client client1 &&\n> \n> And make a clone out of it, without blobs.\n> \n>> +\ttest_commit -C server server2 &&\n>> +\tCOMMIT=$(git -C server rev-parse server2) &&\n> \n> Then we create a new commit that the client does not yet have.\n> \n>> +\ttest_must_fail git -C client push 2>err &&\n> \n> We try to overwrite it.  We expect it to fail with \"not a fast forward\".\n\nWell that is what it would fail with at the moment, but it's not\nwhat would happen with a non-partial clone - a non-partial clone\nwould fail with \"fetch first\" instead.\n\nThis patch makes both cases consistent although that wasn't the\nmain driver - the main driver was to stop it fetching 100Mb or\nmore of history in the large repository I was working with when\nthe upstream has one new commit.\n\n>> +\tgrep \"fetch first\" err &&\n> \n> May want to use \"test_grep\" but this script does not use it, so\n> being consistent with the surrounding tests is good.\n\nSo here we are testing that it's a \"fetch first\" and rather\nthan \"not a fast forward\".\n\n>> +\tgit -C client rev-list --objects --missing=print \"$COMMIT\" >objects &&\n>> +\tgrep \"^[?]$COMMIT\" objects\n>> +'\n> \n> OK.\n\nand also that it hasn't fetched the new commit.\n\nTom\n\n-- \nTom Hughes (tom@compton.nu)\nhttp://compton.nu/\n\n"},{"id":"495337","messageId":"xmqqbk4x1pc4.fsf@gitster.g","threadId":"61530","inReplyTo":"8f2ebf1b-050f-476a-92d4-dfb06ad04f8d@compton.nu","subject":"Re: [PATCH v2] push: don't fetch commit object when checking existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T21:58:19Z","receivedAt":"2024-05-22T21:58:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Hughes <tom@compton.nu> writes:\n\n>>> +\ttest_must_fail git -C client push 2>err &&\n>> We try to overwrite it.  We expect it to fail with \"not a fast\n>> forward\".\n>\n> Well that is what it would fail with at the moment, but it's not\n> what would happen with a non-partial clone - a non-partial clone\n> would fail with \"fetch first\" instead.\n\nOh, don't get me wrong.  I wasn't trying to split hairs between the\ntwo error modes and their phrasing.  The \"fetch-first\" from\nset_ref_status_for_push() is done before we even initiate the\ntransfer to stop the operation, with a cheap check, that will\neventually lead to \"not a fast forward\" error.  IOW, in my mind,\nthey are the same errors, just diagnosed at two different places in\nthe code and their messages phrased differently.\n\n> So here we are testing that it's a \"fetch first\" and rather\n> than \"not a fast forward\".\n\nI think that is being overly specific, but that is fine.  As I said,\nto the end users, these two errors mean the same thing (they would\nneed to fetch first and then integrate their changes before pushing\nit out again), so it is plausible that we may in the future decide\nthat we want to use the same message.  When it happens, this test\nmust change, which may even be a good thing (it makes it clear what\nthe fallout from such a change looks like).\n\n>>> +\tgit -C client rev-list --objects --missing=print \"$COMMIT\" >objects &&\n>>> +\tgrep \"^[?]$COMMIT\" objects\n>>> +'\n>> OK.\n>\n> and also that it hasn't fetched the new commit.\n\nYes, and this is a good check that will stand the test of time, even\nacross a change to rephrase the error message.\n\nThanks.\n\n"},{"id":"495365","messageId":"20240523085814.GA1306938@coredump.intra.peff.net","threadId":"61530","inReplyTo":"20240522201559.1677959-1-tom@compton.nu","subject":"Re: [PATCH v2] push: don't fetch commit object when checking existence","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-05-23T08:58:14Z","receivedAt":"2024-05-23T08:58:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 22, 2024 at 09:15:40PM +0100, Tom Hughes wrote:\n\n> diff --git a/remote.c b/remote.c\n> index 2b650b813b..20395bbbd0 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>  \t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n>  \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n>  \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n> -\t\t\telse if (!repo_has_object_file(the_repository, &ref->old_oid))\n> +\t\t\telse if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n>  \t\t\t\treject_reason = REF_STATUS_REJECT_FETCH_FIRST;\n>  \t\t\telse if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||\n>  \t\t\t\t !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))\n\nThis makes sense to me, as we're just speculatively asking \"do we have\nthe object\". I think for that reason it would also be reasonable to use\nOBJECT_INFO_QUICK here, which would avoid a fruitless re-scan of the\nlocal objects/ directory. We often pair the two[1].\n\nIn practice, though, I think fetching the missing object is going to be\nmuch more expensive than a local re-scan. We tend to notice the latter\nonly when you have a large number of objects to check, and here we're\nbasically limited by the number of non-fast-forward refs you're trying\nto push.\n\nSo I also think it would be OK to leave it here and only do QUICK if\nsomebody ever notices it.\n\n-Peff\n\n[1] We've talked about unifying those two flags, since they so often\n    come together. There's some discussion in:\n\n      https://lore.kernel.org/git/20191011220822.154063-1-jonathantanmy@google.com/\n\n    that they could become one flag, but these two:\n\n      https://lore.kernel.org/git/20190909222101.GB31319@sigill.intra.peff.net/\n\n      https://lore.kernel.org/git/20200322054916.GB578498@coredump.intra.peff.net/\n\n    argue that QUICK implies SKIP_FETCH, but not always the other way\n    around. (Obviously getting a bit off topic for your patch; if\n    anything, I think this call site would just use both for now).\n"}]}