From: Matthew John Cheetham Date: Mon, 18 May 2026 16:57:06 GMT Subject: Re: [PATCH v4 5/8] negotiator: add have_sent() interface Message-ID: In-Reply-To: <94b79784fe6a4f22dba32a2e1d44316b3e84da48.1778762495.git.gitgitgadget@gmail.com> On 2026-05-14 13:41, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee > > In a future change, we will introduce a capability to choose specific commit > OIDs as 'have's in fetch negotiation, with the ability to have the > negotiator choose more 'have's to increase coverage beyond that required > core set. The negotiator works to avoid emitting 'have's that can reach each > other, but that logic is hidden beneath the negotiator's iterator function > pointer ('next'). We need a way to communicate to the negotiator that we > have picked a 'have' so it could incorporate that into its logic. > > Add a have_sent() method to the fetch_negotiator interface. This is the > signal that allows the negotiator to track the commit as already shown and > can perform the proper bookkeeping to avoid emitting those objects or > anything they can reach. > > For our non-trivial negotiators, it is sufficient to mark these commits as > common, so the implementation is quite simple. This logic will be exercised > in the next change. > > Signed-off-by: Derrick Stolee This is a new patch in v4, and is in response to the COMMON (bit 2) vs COMMON (bit 6) issue of v3. Having a defined have_sent() API on the negotiator, and letting the negotiator handle its own book-keeping is much nicer and safer. > --- > fetch-negotiator.h | 9 +++++++++ > negotiator/default.c | 8 ++++++++ > negotiator/noop.c | 7 +++++++ > negotiator/skipping.c | 8 ++++++++ > 4 files changed, 32 insertions(+) > > diff --git a/fetch-negotiator.h b/fetch-negotiator.h > index e348905a1f..6ca422a064 100644 > --- a/fetch-negotiator.h > +++ b/fetch-negotiator.h > @@ -47,6 +47,15 @@ struct fetch_negotiator { > */ > int (*ack)(struct fetch_negotiator *, struct commit *); > > + /* > + * Inform the negotiator that this commit has already been sent as > + * a "have" line outside of the negotiator's control. The negotiator > + * should avoid outputting it from next() and may use it to optimize > + * further negotiation (e.g., by treating it and its ancestors as > + * common). > + */ > + void (*have_sent)(struct fetch_negotiator *, struct commit *); > + Doc comment captures the contract well. > void (*release)(struct fetch_negotiator *); > > /* internal use */ > diff --git a/negotiator/default.c b/negotiator/default.c > index 116dedcf83..05ab616f39 100644 > --- a/negotiator/default.c > +++ b/negotiator/default.c > @@ -175,6 +175,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c) > return known_to_be_common; > } > > +static void have_sent(struct fetch_negotiator *n, struct commit *c) > +{ > + if (repo_parse_commit(the_repository, c)) > + return; > + mark_common(n->data, c, 0, 0); > +} > + > static void release(struct fetch_negotiator *n) > { > clear_prio_queue(&((struct negotiation_state *)n->data)->rev_list); > @@ -188,6 +195,7 @@ void default_negotiator_init(struct fetch_negotiator *negotiator) > negotiator->add_tip = add_tip; > negotiator->next = next; > negotiator->ack = ack; > + negotiator->have_sent = have_sent; > negotiator->release = release; > negotiator->data = CALLOC_ARRAY(ns, 1); > ns->rev_list.compare = compare_commits_by_commit_date; I traced this through default.c's mark_common() to confirm this does what we want.. sets the COMMON bit (bit 2) and propogates COMMON to the ancestors correctly. Good! > diff --git a/negotiator/noop.c b/negotiator/noop.c > index 65e3c20008..edf1b456f3 100644 > --- a/negotiator/noop.c > +++ b/negotiator/noop.c > @@ -29,6 +29,12 @@ static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED) > return 0; > } > > +static void have_sent(struct fetch_negotiator *n UNUSED, > + struct commit *c UNUSED) > +{ > + /* nothing to do */ > +} > + > static void release(struct fetch_negotiator *n UNUSED) > { > /* nothing to release */ > @@ -40,6 +46,7 @@ void noop_negotiator_init(struct fetch_negotiator *negotiator) > negotiator->add_tip = add_tip; > negotiator->next = next; > negotiator->ack = ack; > + negotiator->have_sent = have_sent; > negotiator->release = release; > negotiator->data = NULL; > } Trivial implementation of the noop negotiator. Good. > diff --git a/negotiator/skipping.c b/negotiator/skipping.c > index 0a272130fb..69472c58e1 100644 > --- a/negotiator/skipping.c > +++ b/negotiator/skipping.c > @@ -243,6 +243,13 @@ static int ack(struct fetch_negotiator *n, struct commit *c) > return known_to_be_common; > } > > +static void have_sent(struct fetch_negotiator *n, struct commit *c) > +{ > + if (repo_parse_commit(the_repository, c)) > + return; > + mark_common(n->data, c); > +} > + > static void release(struct fetch_negotiator *n) > { > struct data *data = n->data; > @@ -259,6 +266,7 @@ void skipping_negotiator_init(struct fetch_negotiator *negotiator) > negotiator->add_tip = add_tip; > negotiator->next = next; > negotiator->ack = ack; > + negotiator->have_sent = have_sent; > negotiator->release = release; > negotiator->data = CALLOC_ARRAY(data, 1); > data->rev_list.compare = compare; I traced this too, to skipping.c's mark_common(), and it sets the COMMON bit correctly. This patch is a welcome addition and LGTM! Thanks, Matthew