From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 5/7] fetch-pack: do not lazy-fetch during ref iteration
Date: Wed, 12 Aug 2020 11:25:38 -0700 [thread overview]
Message-ID: <xmqqft8r7mcd.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <d42e98ff0760932cffb021eb680b76da74e4461f.1597184949.git.jonathantanmy@google.com> (Jonathan Tan's message of "Tue, 11 Aug 2020 15:52:20 -0700")
Jonathan Tan <jonathantanmy@google.com> writes:
> Teach "fetch-pack" not to lazy fetch whenever iterating over
> refs.
Don't readers need the reason why is it a good thing to do explained
here?
> This
> is done by using the raw form of ref iteration and by dereferencing tags
> ourselves.
Hmph. The way this patch implements deref_without_lazy_fetch()
makes it hard to reuse in other contexts, as it mixes what is done
by mark_complete(), which is very much "git fetch" specific.
Would "git fetch" be the only user potentially benefit from being
able to dereference a tag that already exists locally and ignore
missing ones? I wonder if teaching deref_tag() a new flag would be
a better alternative to keep the separation of concerns at different
layers.
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> fetch-pack.c | 79 ++++++++++++++++++++++------------------
> t/t5616-partial-clone.sh | 20 ++++++++++
> 2 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 80fb3bd899..707bbc31fd 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -108,24 +108,48 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
> cb(negotiator, cache.items[i]);
> }
>
> +static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> + int mark_tags_complete)
> +{
> + enum object_type type;
> + struct object_info info = { .typep = &type };
> +
> + while (1) {
> + if (oid_object_info_extended(the_repository, oid, &info,
> + OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
> + return NULL;
> + if (type == OBJ_TAG) {
> + struct tag *tag = (struct tag *)
> + parse_object(the_repository, oid);
> +
> + if (!tag->tagged)
> + return NULL;
> + if (mark_tags_complete)
> + tag->object.flags |= COMPLETE;
> + oid = &tag->tagged->oid;
> + } else {
> + break;
> + }
> + }
> + if (type == OBJ_COMMIT)
> + return (struct commit *) parse_object(the_repository, oid);
> + return NULL;
> +}
> +
> static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
> - const char *refname,
> const struct object_id *oid)
> {
> - struct object *o = deref_tag(the_repository,
> - parse_object(the_repository, oid),
> - refname, 0);
> -
> - if (o && o->type == OBJ_COMMIT)
> - negotiator->add_tip(negotiator, (struct commit *)o);
> + struct commit *c = deref_without_lazy_fetch(oid, 0);
>
> + if (c)
> + negotiator->add_tip(negotiator, c);
> return 0;
> }
>
> static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,
> int flag, void *cb_data)
> {
> - return rev_list_insert_ref(cb_data, refname, oid);
> + return rev_list_insert_ref(cb_data, oid);
> }
>
> enum ack_type {
> @@ -201,7 +225,7 @@ static void send_request(struct fetch_pack_args *args,
> static void insert_one_alternate_object(struct fetch_negotiator *negotiator,
> struct object *obj)
> {
> - rev_list_insert_ref(negotiator, NULL, &obj->oid);
> + rev_list_insert_ref(negotiator, &obj->oid);
> }
>
> #define INITIAL_FLUSH 16
> @@ -230,13 +254,12 @@ static void mark_tips(struct fetch_negotiator *negotiator,
> int i;
>
> if (!negotiation_tips) {
> - for_each_ref(rev_list_insert_ref_oid, negotiator);
> + for_each_rawref(rev_list_insert_ref_oid, negotiator);
> return;
> }
>
> for (i = 0; i < negotiation_tips->nr; i++)
> - rev_list_insert_ref(negotiator, NULL,
> - &negotiation_tips->oid[i]);
> + rev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);
> return;
> }
>
> @@ -503,21 +526,11 @@ static struct commit_list *complete;
>
> static int mark_complete(const struct object_id *oid)
> {
> - struct object *o = parse_object(the_repository, oid);
> -
> - while (o && o->type == OBJ_TAG) {
> - struct tag *t = (struct tag *) o;
> - if (!t->tagged)
> - break; /* broken repository */
> - o->flags |= COMPLETE;
> - o = parse_object(the_repository, &t->tagged->oid);
> - }
> - if (o && o->type == OBJ_COMMIT) {
> - struct commit *commit = (struct commit *)o;
> - if (!(commit->object.flags & COMPLETE)) {
> - commit->object.flags |= COMPLETE;
> - commit_list_insert(commit, &complete);
> - }
> + struct commit *commit = deref_without_lazy_fetch(oid, 1);
> +
> + if (commit && !(commit->object.flags & COMPLETE)) {
> + commit->object.flags |= COMPLETE;
> + commit_list_insert(commit, &complete);
> }
> return 0;
> }
> @@ -702,7 +715,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
> */
> trace2_region_enter("fetch-pack", "mark_complete_local_refs", NULL);
> if (!args->deepen) {
> - for_each_ref(mark_complete_oid, NULL);
> + for_each_rawref(mark_complete_oid, NULL);
> for_each_cached_alternate(NULL, mark_alternate_complete);
> commit_list_sort_by_date(&complete);
> if (cutoff)
> @@ -716,16 +729,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
> */
> trace2_region_enter("fetch-pack", "mark_common_remote_refs", NULL);
> for (ref = *refs; ref; ref = ref->next) {
> - struct object *o = deref_tag(the_repository,
> - lookup_object(the_repository,
> - &ref->old_oid),
> - NULL, 0);
> + struct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);
>
> - if (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))
> + if (!c || !(c->object.flags & COMPLETE))
> continue;
>
> - negotiator->known_common(negotiator,
> - (struct commit *)o);
> + negotiator->known_common(negotiator, c);
> }
> trace2_region_leave("fetch-pack", "mark_common_remote_refs", NULL);
>
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8a27452a51..e53492d595 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -384,6 +384,26 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
> grep "want $(cat hash)" trace
> '
>
> +test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '
> + rm -rf server client trace &&
> +
> + test_create_repo server &&
> + test_config -C server uploadpack.allowfilter 1 &&
> + test_config -C server uploadpack.allowanysha1inwant 1 &&
> + test_commit -C server foo &&
> +
> + git clone --filter=blob:none "file://$(pwd)/server" client &&
> + # Make all refs point to nothing by deleting all objects.
> + rm client/.git/objects/pack/* &&
> +
> + test_commit -C server bar &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> + --no-tags --recurse-submodules=no \
> + origin refs/tags/bar &&
> + FOO_HASH=$(git -C server rev-parse foo) &&
> + ! grep "want $FOO_HASH" trace
> +'
> +
> # The following two tests must be in this order. It is important that
> # the srv.bare repository did not have tags during clone, but has tags
> # in the fetch.
next prev parent reply other threads:[~2020-08-12 18:25 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-24 22:38 [RFC PATCH] Modify fetch-pack to no longer die on error? Jonathan Tan
2020-07-24 23:07 ` Junio C Hamano
2020-07-24 23:11 ` Junio C Hamano
2020-07-25 21:41 ` Jeff King
2020-07-25 23:01 ` Junio C Hamano
2020-07-27 17:11 ` Jeff King
2020-07-28 19:23 ` Jonathan Tan
2020-07-28 20:08 ` Jeff King
2020-07-29 18:53 ` Jonathan Tan
2020-07-29 19:29 ` Jeff King
2020-07-29 19:02 ` Junio C Hamano
2020-07-29 22:55 ` Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 0/7] Lazy fetch with subprocess Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip Jonathan Tan
2020-08-05 19:53 ` Junio C Hamano
2020-08-07 20:53 ` Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common Jonathan Tan
2020-08-05 20:08 ` Junio C Hamano
2020-08-05 22:11 ` Junio C Hamano
2020-08-07 20:59 ` Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 3/7] negotiator/null: add null fetch negotiator Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 4/7] fetch: --stdin Jonathan Tan
2020-08-05 20:33 ` Junio C Hamano
2020-08-07 21:10 ` Jonathan Tan
2020-08-07 21:58 ` Junio C Hamano
2020-08-07 21:10 ` Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 5/7] fetch: submodule config Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 6/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-05 1:20 ` [RFC PATCH 7/7] promisor-remote: use subprocess to fetch Jonathan Tan
2020-08-11 22:52 ` [PATCH v2 0/7] Lazy fetch with subprocess Jonathan Tan
2020-08-11 22:52 ` [PATCH v2 1/7] negotiator/null: add null fetch negotiator Jonathan Tan
2020-08-12 12:55 ` Derrick Stolee
2020-08-12 16:44 ` Junio C Hamano
2020-08-12 17:29 ` Jonathan Tan
2020-08-11 22:52 ` [PATCH v2 2/7] fetch: allow refspecs specified through stdin Jonathan Tan
2020-08-11 22:52 ` [PATCH v2 3/7] fetch: avoid reading submodule config until needed Jonathan Tan
2020-08-12 17:34 ` Junio C Hamano
2020-08-11 22:52 ` [PATCH v2 4/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-12 18:06 ` Junio C Hamano
2020-08-11 22:52 ` [PATCH v2 5/7] fetch-pack: do not lazy-fetch during ref iteration Jonathan Tan
2020-08-12 18:25 ` Junio C Hamano [this message]
2020-08-11 22:52 ` [PATCH v2 6/7] promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-12 18:28 ` Junio C Hamano
2020-08-11 22:52 ` [PATCH v2 7/7] fetch-pack: remove no_dependents code Jonathan Tan
2020-08-12 12:51 ` [PATCH v2 0/7] Lazy fetch with subprocess Derrick Stolee
2020-08-18 4:01 ` [PATCH v3 " Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 1/7] negotiator/noop: add noop fetch negotiator Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 2/7] fetch: allow refspecs specified through stdin Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 3/7] fetch: avoid reading submodule config until needed Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 4/7] fetch: only populate existing_refs if needed Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 5/7] fetch-pack: do not lazy-fetch during ref iteration Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 6/7] promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-18 4:01 ` [PATCH v3 7/7] fetch-pack: remove no_dependents code Jonathan Tan
2020-08-18 19:56 ` [PATCH v3 0/7] Lazy fetch with subprocess Junio C Hamano
2020-08-18 22:32 ` Junio C Hamano
2020-08-18 23:36 ` [PATCH] fixup! promisor-remote: lazy-fetch objects in subprocess Jonathan Tan
2020-08-18 23:57 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqft8r7mcd.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).