git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Jun 18, 2019, 17:30 UTC
Message-ID
<20190618173006.182337-1-jonathantanmy@google.com>
In-Reply-To
<87o92v817k.fsf@evledraar.gmail.com>
Show 47 quoted lines
> > @@ -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.

Previous: Ævar Arnfjörð Bjarmason
Message 14 of 14 in “fetch-pack: support negotiation tip whitelist”
  1. fetch-pack: support negotiation tip whitelistJonathan Tan, Jun 25, 2018
  2. Jonathan NiederJun 26, 2018
  3. Junio C HamanoJun 26, 2018
  4. fetch-pack: support negotiation tip whitelistJonathan Tan, Jun 27, 2018
  5. Brandon WilliamsJun 28, 2018
  6. Jonathan TanJun 28, 2018
  7. Brandon WilliamsJun 28, 2018
  8. fetch-pack: support negotiation tip whitelistJonathan Tan, Jun 28, 2018
  9. Brandon WilliamsJun 28, 2018
  10. Junio C HamanoJun 29, 2018
  11. fetch-pack: support negotiation tip whitelistJonathan Tan, Jul 2, 2018
  12. Duy NguyenJul 22, 2018
  13. Ævar Arnfjörð BjarmasonJun 18, 2019
  14. Jonathan TanJun 18, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.