git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v3 2/5] fetch-pack: load tip_oids eagerly iff needed
Date: Fri, 5 Oct 2018 23:22:31 +0200	[thread overview]
Message-ID: <a5a5b0c5-cc97-dc50-56dd-25ec64c03e48@web.de> (raw)
In-Reply-To: <20181005202737.GA22460@sigill.intra.peff.net>

Am 05.10.2018 um 22:27 schrieb Jeff King:
> On Fri, Oct 05, 2018 at 10:13:34PM +0200, René Scharfe wrote:
> 
>>>> -{
>>>> -	/*
>>>> -	 * Note that this only looks at the ref lists the first time it's
>>>> -	 * called. This works out in filter_refs() because even though it may
>>>> -	 * add to "newlist" between calls, the additions will always be for
>>>> -	 * oids that are already in the set.
>>>> -	 */
>>>
>>> I don't think the subtle point this comment is making goes away. We're
>>> still growing the list in the loop that calls tip_oids_contain() (and
>>> which now calls just oidset_contains). That's OK for the reasons given
>>> here, but I think that would need to be moved down to this code:
>>>
>>>> +	if (strict) {
>>>> +		for (i = 0; i < nr_sought; i++) {
>>>> +			ref = sought[i];
>>>> +			if (!is_unmatched_ref(ref))
>>>> +				continue;
>>>> +
>>>> +			add_refs_to_oidset(&tip_oids, unmatched);
>>>> +			add_refs_to_oidset(&tip_oids, newlist);
>>>> +			break;
>>>> +		}
>>>> +	}
>>>
>>> I.e., we need to say here why it's OK to summarize newlist in the
>>> oidset, even though we're adding to it later.
>>
>> There is already this comment:
>>
>> 	/* Append unmatched requests to the list */
>>
>> And that's enough in my eyes.  The refs loop at the top splits the list
>> into matched ("the list") and unmatched, and the loop below said comment
>> adds a few more.  I see no subtlety left -- what do I miss?
> 
> It looks like tip_oids is meant as a fast lookup into what's in
> unmatched and newlist. But in the second loop we continue appending to
> newlist. Why is it OK that we do not update tip_oids when we do so?

`tip_oids` contains the object_ids of the all `refs` passed to
filter_refs().  Instead of adding them at the top of the function that
is done only when it has become clear that there are unmatched ones,
as an optimization.  (That optimization was implemented by lazy-loading
in tip_oids_contain() earlier.)  At that point the list has been split
into `newlist` and `unmatched`, so we load from them instead of `refs`.

> 
> I.e., something like this explains it:
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 53914563b5..c0a1b80f4c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -606,6 +606,12 @@ static void filter_refs(struct fetch_pack_args *args,
>  			ref->match_status = REF_MATCHED;
>  			*newtail = copy_ref(ref);
>  			newtail = &(*newtail)->next;
> +			/*
> +			 * No need to update tip_oids with ref->old_oid; we got
> +			 * here because either it was already there, or we are
> +			 * in !strict mode, in which case we do not use
> +			 * tip_oids at all.
> +			 */
>  		} else {
>  			ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
>  		}

This comment puzzles me -- why ask the question it answers?
`tip_oids` has been loaded with all `refs` at that point; adding
more seems odd.

I feel the code needs be simplified further; not sure how, though,
except perhaps by using the `unfound` array added in another reply.

René

  reply	other threads:[~2018-10-05 21:22 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-03 13:11 [PATCH v2 0/2] oidset: use khash René Scharfe
2018-10-03 13:12 ` [PATCH v2 1/2] khash: factor out kh_release_* René Scharfe
2018-10-03 13:16 ` [PATCH v2 2/2] oidset: use khash René Scharfe
2018-10-03 19:40   ` Jeff King
2018-10-04  5:56     ` René Scharfe
2018-10-04  6:48       ` Jeff King
2018-10-04  6:50         ` Jeff King
2018-10-04 15:05         ` René Scharfe
2018-10-04 15:05 ` [PATCH v3 0/5] " René Scharfe
2018-10-04 15:09   ` [PATCH v3 1/5] fetch-pack: factor out is_unmatched_ref() René Scharfe
2018-10-04 15:09   ` [PATCH v3 2/5] fetch-pack: load tip_oids eagerly iff needed René Scharfe
2018-10-04 21:38     ` Jonathan Tan
2018-10-04 22:11       ` René Scharfe
2018-10-05 20:13         ` René Scharfe
2018-10-04 22:14       ` Jeff King
2018-10-04 22:52         ` Jonathan Tan
2018-10-04 23:18           ` Jeff King
2018-10-04 22:07     ` Jeff King
2018-10-05 20:13       ` René Scharfe
2018-10-05 20:27         ` Jeff King
2018-10-05 21:22           ` René Scharfe [this message]
2018-10-05 21:47             ` Jeff King
2018-10-04 15:10   ` [PATCH v3 3/5] khash: factor out kh_release_* René Scharfe
2018-10-04 15:13   ` [PATCH v3 4/5] oidset: use khash René Scharfe
2018-10-04 15:14   ` [PATCH 5/5] oidset: uninline oidset_init() René Scharfe
2018-10-04 22:15   ` [PATCH v3 0/5] oidset: use khash Jeff King

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a5a5b0c5-cc97-dc50-56dd-25ec64c03e48@web.de \
    --to=l.s.r@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v3 2/5] fetch-pack: load tip_oids eagerly iff needed' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox