Re: [PATCH v4] fetch-pack: support negotiation tip whitelist

[Jonathan Tan <jonathantanmy@google.com>]

> > @@ -230,7 +246,7 @@ static int find_common(struct fetch_negotiator *negotiator,
> >  	if (args->stateless_rpc && multi_ack == 1)
> >  		die(_("--stateless-rpc requires multi_ack_detailed"));
> >
> > -	for_each_ref(rev_list_insert_ref_oid, negotiator);
> > +	mark_tips(negotiator, args->negotiation_tips);
> >  	for_each_cached_alternate(negotiator, insert_one_alternate_object);
> >
> >  	fetching = 0;
> 
> Here we blindly add objects found in an alternate repo. I found and
> debugged this with this:
> 
>     diff --git a/fetch-negotiator.h b/fetch-negotiator.h
>     index 9e3967ce66..cbe71c9c8d 100644
>     --- a/fetch-negotiator.h
>     +++ b/fetch-negotiator.h
>     @@ -33,2 +33,3 @@ struct fetch_negotiator {
>             void (*add_tip)(struct fetch_negotiator *, struct commit *);
>     +       int done_adding;
> 
>     diff --git a/fetch-pack.c b/fetch-pack.c
>     index 3f24d0c8a6..6b43b4f8f1 100644
>     --- a/fetch-pack.c
>     +++ b/fetch-pack.c
>     @@ -238,2 +238,3 @@ static void mark_tips(struct fetch_negotiator *negotiator,
>                                         &negotiation_tips->oid[i]);
>     +       negotiator->done_adding = 1;
>             return;
>     diff --git a/negotiator/default.c b/negotiator/default.c
>     index 4b78f6bf36..4e45f05f25 100644
>     --- a/negotiator/default.c
>     +++ b/negotiator/default.c
>     @@ -137,2 +137,4 @@ static void add_tip(struct fetch_negotiator *n, struct commit *c)
>      {
>     +       if (n->done_adding)
>     +               return;
>             n->known_common = NULL;
>     @@ -166,2 +168,3 @@ void default_negotiator_init(struct fetch_negotiator *negotiator)
>             negotiator->add_tip = add_tip;
>     +       negotiator->done_adding = 0;
>             negotiator->next = next;
> 
> Perhaps something like that with an assert() is a good idea for the
> negotiation backend code in general? It seems rather fragile to depend
> on there being no other codepath that calls add_tip() again after some
> other code (--negotiation-tip=*) that expects it not to be called again.

Thanks for spotting this bug.

There is already some defense from add_tip() not being called
unexpectedly - see negotiator/default.c and negotiator.skipping.c, which
sets add_tip to NULL when next() is called.

I can see that this doesn't help in this case, when we want to declare
done_adding but we haven't called next() yet, but I don't think that
this API layer is the right place to prevent that.