diff mbox series

push: don't fetch commit object when checking existence

Message ID 20240522133621.1308393-1-tom@compton.nu (mailing list archive)
State Superseded
Headers show
Series push: don't fetch commit object when checking existence | expand

Commit Message

Tom Hughes May 22, 2024, 1:36 p.m. UTC
If we're checking to see whether to tell the user to do a fetch
before pushing there's no need for us to actually fetch the object
from the remote if the clone is partial.

Because the promisor doesn't do negotiation actually trying to do
the fetch of the new head can be very expensive as it will try and
include history that we already have and it just results in rejecting
the push with a different message, and in behavior that is different
to a clone that is not partial.

Signed-off-by: Tom Hughes <tom@compton.nu>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano May 22, 2024, 7:16 p.m. UTC | #1
Tom Hughes <tom@compton.nu> writes:

> If we're checking to see whether to tell the user to do a fetch
> before pushing there's no need for us to actually fetch the object
> from the remote if the clone is partial.
>
> Because the promisor doesn't do negotiation actually trying to do
> the fetch of the new head can be very expensive as it will try and
> include history that we already have and it just results in rejecting
> the push with a different message, and in behavior that is different
> to a clone that is not partial.

Interesting.  Is this something that is easily testable, perhaps by
preparing a partial clone and try to push from there and checking
the non-existence of the object after seeing that push failed?

Thanks.

> Signed-off-by: Tom Hughes <tom@compton.nu>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 2b650b813b..20395bbbd0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  		if (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {
>  			if (starts_with(ref->name, "refs/tags/"))
>  				reject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;
> -			else if (!repo_has_object_file(the_repository, &ref->old_oid))
> +			else if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))
>  				reject_reason = REF_STATUS_REJECT_FETCH_FIRST;
>  			else if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||
>  				 !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))
Tom Hughes May 22, 2024, 8:18 p.m. UTC | #2
On 22/05/2024 20:16, Junio C Hamano wrote:
> Tom Hughes <tom@compton.nu> writes:
> 
>> If we're checking to see whether to tell the user to do a fetch
>> before pushing there's no need for us to actually fetch the object
>> from the remote if the clone is partial.
>>
>> Because the promisor doesn't do negotiation actually trying to do
>> the fetch of the new head can be very expensive as it will try and
>> include history that we already have and it just results in rejecting
>> the push with a different message, and in behavior that is different
>> to a clone that is not partial.
> 
> Interesting.  Is this something that is easily testable, perhaps by
> preparing a partial clone and try to push from there and checking
> the non-existence of the object after seeing that push failed?

Sure. I think I've managed to figure out a test and have sent
a second version of the patch with it added.

Tom
diff mbox series

Patch

diff --git a/remote.c b/remote.c
index 2b650b813b..20395bbbd0 100644
--- a/remote.c
+++ b/remote.c
@@ -1773,7 +1773,7 @@  void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		if (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {
 			if (starts_with(ref->name, "refs/tags/"))
 				reject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;
-			else if (!repo_has_object_file(the_repository, &ref->old_oid))
+			else if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))
 				reject_reason = REF_STATUS_REJECT_FETCH_FIRST;
 			else if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||
 				 !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))