Re: [PATCH v2] negotiator/skipping.c: parse parents before queueing

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <ttaylorr@openai.com> writes:

> From: Taylor Blau <ttaylorr@openai.com>
>
> The skipping negotiator sorts its queue by commit date, but callers
> which push commits onto that queue, such as `push_parent()`, can add a
> parent before parsing it. When an unparsed commit enters the queue, its
> date is still zero, so old ref tips may sort ahead of it.
>
> For example, suppose a topic branch is one commit ahead of master.
> Skipping offers the topic tip and skips master's tip. Master's parent
> then waits behind old ref tips. With enough old refs, negotiation can
> end before we offer that recent common ancestor.
>
> Parse each unseen parent before adding it to the queue, so the queue
> uses its actual date. If parsing fails, leave the parent out of the
> queue.
>
> Since the sole caller of `prio_queue_put()` ensures that commits are
> fully parsed before pushing them onto the queue, we can avoid a noop
> call to `repo_parse_commit()` within `get_rev()`.
>
> Note that unlike the default negotiator, the skipping negotiator's
> version of `rev_list_push()` returns a pointer to an 'entry' struct. As
> a consequence, callers must be prepared to handle a NULL return value.
> The caller in `push_parent()` gains a NULL-ness check, and the remaining
> two callers continue to ignore the return value from `rev_list_push()`.
>
> Signed-off-by: Taylor Blau <ttaylorr@openai.com>
> ---
>  negotiator/skipping.c                |  8 +++++--
>  t/t5552-skipping-fetch-negotiator.sh | 32 ++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> index db90fa77b53..0ae5f28d353 100644
> --- a/negotiator/skipping.c
> +++ b/negotiator/skipping.c
> @@ -66,6 +66,9 @@ static struct entry *rev_list_push(struct data *data, struct commit *commit, int
>  	struct entry *entry;
>  	commit->object.flags |= mark | SEEN;
>  
> +	if (repo_parse_commit(the_repository, commit))
> +		return NULL;
> +
>  	CALLOC_ARRAY(entry, 1);
>  	entry->commit = commit;
>  	prio_queue_put(&data->rev_list, entry);
> @@ -125,7 +128,7 @@ static void mark_common(struct data *data, struct commit *seen_commit)
>   * entry has the correct flags and ttl.
>   *
>   * This function returns 1 if an entry was found or created, and 0 otherwise
> - * (because the entry for this commit had already been popped).
> + * (because the commit could not be parsed or its entry had already been popped).
>   */
>  static int push_parent(struct data *data, struct entry *entry,
>  		       struct commit *to_push)
> @@ -152,6 +155,8 @@ static int push_parent(struct data *data, struct entry *entry,
>  		;
>  	} else {
>  		parent_entry = rev_list_push(data, to_push, 0);
> +		if (!parent_entry)
> +			return 0;
>  	}
>  
>  	if (entry->commit->object.flags & (COMMON | ADVERTISED)) {
> @@ -194,7 +199,6 @@ static const struct object_id *get_rev(struct data *data)
>  		if (!(commit->object.flags & COMMON) && !entry->ttl)
>  			to_send = commit;
>  
> -		repo_parse_commit(the_repository, commit);
>  		for (p = commit->parents; p; p = p->next)
>  			parent_pushed |= push_parent(data, entry, p->item);

Moving the parsing to rev_list_push() means that when a commit
cannot be parsed, it is marked as 'SEEN' and NULL is returned.  The
caller, push_parent(), notices this NULL return and naturally
returns without pushing the parent.  However, when the same
unparsable commit is encountered again (perhaps as a parent of
another commit), push_parent() sees that it is already 'SEEN' and
looks for it in the priority queue.  It will fail to find it and
trigger BUG("missing parent in priority queue").

Perhaps rev_list_push() should mark the commit as 'SEEN' only after
repo_parse_commit() succeeds?

While this issue only arises with a corrupt repository or when a
promisor remote goes offline, a failure to parse should not trigger
a BUG(), which should be reserved for programming errors.

Thanks.