promisor-remote: try accepted remotes before others in get_direct()

When a server advertises promisor remotes and the client accepts some of them, those remotes carry the server's intent: 'fetch missing objects preferably from here', and the client agrees with that for the remotes it accepts. However promisor_remote_get_direct() actually iterates over all promisor remotes in list order, which is the order they appear in the config files (except perhaps for the one appearing in the `extensions.partialClone` config variable which is tried last). This means an existing, but not accepted, promisor remote, could be tried before the accepted ones, which does not reflect the intent of the agreement between client and server. If the client doesn't care about what the server suggests, it should accept nothing and rely on its remotes as they are already configured. To better reflect the agreement between client and server, let's make promisor_remote_get_direct() try the accepted promisor remotes before the non-accepted ones. Concretely, let's extract a try_promisor_remotes() helper and call it twice from promisor_remote_get_direct(): - first with an `accepted_only=true` argument to try only the accepted remotes, - then with `accepted_only=false` to fall back to any remaining remote. Ensuring that accepted remotes are preferred will be even more important if in the future a mechanism is developed to allow the client to auto-configure remotes that the server advertises. This will in particular avoid fetching from the server (which is already configured as a promisor remote) before trying the auto-configured remotes, as these new remotes would likely appear at the end of the config file, and as the server might not appear in the `extensions.partialClone` config variable. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Christian Couder committed Apr 7, 2026 at 13:52 UTC 8808e61fd3e953c3534633b8b5adc5b243dd696f
3 files changed +104 -13
Documentation/gitprotocol-v2.adoc
+4
@@ -848,6 +848,10 @@ advertised, it can reply with "promisor-remote=<pr-names>" where
848 where `pr-name` is the urlencoded name of a promisor remote the server
849 advertised and the client accepts.
850
851 +The promisor remotes that the client accepted will be tried before the
852 +other configured promisor remotes when the client attempts to fetch
853 +missing objects.
854 +
855 Note that, everywhere in this document, the ';' and ',' characters
856 MUST be encoded if they appear in `pr-name` or `field-value`.
857
promisor-remote.c
+31 -13
@@ -268,11 +268,35 @@ static int remove_fetched_oids(struct repository *repo,
268 return remaining_nr;
269 }
270
271 +static int try_promisor_remotes(struct repository *repo,
272 + struct object_id **remaining_oids,
273 + int *remaining_nr, int *to_free,
274 + bool accepted_only)
275 +{
276 + struct promisor_remote *r = repo->promisor_remote_config->promisors;
277 +
278 + for (; r; r = r->next) {
279 + if (accepted_only != r->accepted)
280 + continue;
281 + if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
282 + if (*remaining_nr == 1)
283 + continue;
284 + *remaining_nr = remove_fetched_oids(repo, remaining_oids,
285 + *remaining_nr, *to_free);
286 + if (*remaining_nr) {
287 + *to_free = 1;
288 + continue;
289 + }
290 + }
291 + return 1; /* all fetched */
292 + }
293 + return 0;
294 +}
295 +
296 void promisor_remote_get_direct(struct repository *repo,
297 const struct object_id *oids,
298 int oid_nr)
299 {
275 - struct promisor_remote *r;
300 struct object_id *remaining_oids = (struct object_id *)oids;
301 int remaining_nr = oid_nr;
302 int to_free = 0;
@@ -283,19 +307,13 @@ void promisor_remote_get_direct(struct repository *repo,
307
308 promisor_remote_init(repo);
309
286 - for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
287 - if (fetch_objects(repo, r->name, remaining_oids, remaining_nr) < 0) {
288 - if (remaining_nr == 1)
289 - continue;
290 - remaining_nr = remove_fetched_oids(repo, &remaining_oids,
291 - remaining_nr, to_free);
292 - if (remaining_nr) {
293 - to_free = 1;
294 - continue;
295 - }
296 - }
310 + /* Try accepted remotes first (those the server told us to use) */
311 + if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
312 + &to_free, true))
313 + goto all_fetched;
314 + if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
315 + &to_free, false))
316 goto all_fetched;
298 - }
317
318 for (i = 0; i < remaining_nr; i++) {
319 if (is_promisor_object(repo, &remaining_oids[i]))
t/t5710-promisor-remote-capability.sh
+69
@@ -166,6 +166,75 @@ test_expect_success "init + fetch with promisor.advertise set to 'true'" '
166 check_missing_objects server 1 "$oid"
167 '
168
169 +test_expect_success "clone with two promisors but only one advertised" '
170 + git -C server config promisor.advertise true &&
171 + test_when_finished "rm -rf client unused_lop" &&
172 +
173 + # Create a promisor that will be configured but not be used
174 + git init --bare unused_lop &&
175 +
176 + # Clone from server to create a client
177 + GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
178 + -c remote.unused_lop.promisor=true \
179 + -c remote.unused_lop.fetch="+refs/heads/*:refs/remotes/unused_lop/*" \
180 + -c remote.unused_lop.url="file://$(pwd)/unused_lop" \
181 + -c remote.lop.promisor=true \
182 + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
183 + -c remote.lop.url="file://$(pwd)/lop" \
184 + -c promisor.acceptfromserver=All \
185 + --no-local --filter="blob:limit=5k" server client &&
186 +
187 + # Check that "unused_lop" appears before "lop" in the config
188 + printf "remote.%s.promisor true\n" "unused_lop" "lop" "origin" >expect &&
189 + git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
190 + test_cmp expect actual &&
191 +
192 + # Check that "lop" was tried
193 + test_grep " fetch lop " trace &&
194 + # Check that "unused_lop" was not contacted
195 + # This means "lop", the accepted promisor, was tried first
196 + test_grep ! " fetch unused_lop " trace &&
197 +
198 + # Check that the largest object is still missing on the server
199 + check_missing_objects server 1 "$oid"
200 +'
201 +
202 +test_expect_success "init + fetch two promisors but only one advertised" '
203 + git -C server config promisor.advertise true &&
204 + test_when_finished "rm -rf client unused_lop" &&
205 +
206 + # Create a promisor that will be configured but not be used
207 + git init --bare unused_lop &&
208 +
209 + mkdir client &&
210 + git -C client init &&
211 + git -C client config remote.unused_lop.promisor true &&
212 + git -C client config remote.unused_lop.fetch "+refs/heads/*:refs/remotes/unused_lop/*" &&
213 + git -C client config remote.unused_lop.url "file://$(pwd)/unused_lop" &&
214 + git -C client config remote.lop.promisor true &&
215 + git -C client config remote.lop.fetch "+refs/heads/*:refs/remotes/lop/*" &&
216 + git -C client config remote.lop.url "file://$(pwd)/lop" &&
217 + git -C client config remote.server.url "file://$(pwd)/server" &&
218 + git -C client config remote.server.fetch "+refs/heads/*:refs/remotes/server/*" &&
219 + git -C client config promisor.acceptfromserver All &&
220 +
221 + # Check that "unused_lop" appears before "lop" in the config
222 + printf "remote.%s.promisor true\n" "unused_lop" "lop" >expect &&
223 + git -C client config get --all --show-names --regexp "^remote\..*\.promisor$" >actual &&
224 + test_cmp expect actual &&
225 +
226 + GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git -C client fetch --filter="blob:limit=5k" server &&
227 +
228 + # Check that "lop" was tried
229 + test_grep " fetch lop " trace &&
230 + # Check that "unused_lop" was not contacted
231 + # This means "lop", the accepted promisor, was tried first
232 + test_grep ! " fetch unused_lop " trace &&
233 +
234 + # Check that the largest object is still missing on the server
235 + check_missing_objects server 1 "$oid"
236 +'
237 +
238 test_expect_success "clone with promisor.acceptfromserver set to 'KnownName'" '
239 git -C server config promisor.advertise true &&
240 test_when_finished "rm -rf client" &&