| Message ID | 006e89f384be1227b922fb6fdc8755ae84cac587.1666967670.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | receive-pack: use advertised reference tips to inform connectivity check | expand |
On Fri, Oct 28 2022, Patrick Steinhardt wrote: > When serving a push, git-receive-pack(1) needs to verify that the > packfile sent by the client contains all objects that are required by > the updated references. This connectivity check works by marking all > preexisting references as uninteresting and using the new reference tips > as starting point for a graph walk. > > This strategy has the major downside that it will not require any object > to be sent by the client that is reachable by any of the repositories' > references. While that sounds like it would be indeed what we are after > with the connectivity check, it is arguably not. The administrator that > manages the server-side Git repository may have configured certain refs > to be hidden during the reference advertisement via `transfer.hideRefs` > or `receivepack.hideRefs`. Whatever the reason, the result is that the > client shouldn't expect that any of those hidden references exists on > the remote side, and neither should they assume any of the pointed-to > objects to exist except if referenced by any visible reference. But > because we treat _all_ local refs as uninteresting in the connectivity > check, a client is free to send a packfile that references objects that > are only reachable via a hidden reference on the server-side, and we > will gladly accept it. > > Besides the correctness issue there is also a performance issue. Git > forges tend to do internal bookkeeping to keep alive sets of objects for > internal use or make them easy to find via certain references. These > references are typically hidden away from the user so that they are > neither advertised nor writeable. At GitLab, we have one particular > repository that contains a total of 7 million references, of which 6.8 > million are indeed internal references. With the current connectivity > check we are forced to load all these references in order to mark them > as uninteresting, and this alone takes around 15 seconds to compute. > > We can fix both of these issues by changing the logic for stateful > invocations of git-receive-pack(1) where the reference advertisement and > packfile negotiation are served by the same process. Instead of marking > all preexisting references as unreachable, we will only mark those that > we have announced to the client. > > Besides the stated fix to correctness this also provides a huge boost to > performance in the repository mentioned above. Pushing a new commit into > this repo with `transfer.hideRefs` set up to hide 6.8 million of 7 refs > as it is configured in Gitaly leads to an almost 7.5-fold speedup: Really well explained. > Benchmark 1: main > Time (mean ± σ): 29.902 s ± 0.105 s [User: 29.176 s, System: 1.052 s] > Range (min … max): 29.781 s … 29.969 s 3 runs > > Benchmark 2: pks-connectivity-check-hide-refs > Time (mean ± σ): 4.033 s ± 0.088 s [User: 4.071 s, System: 0.374 s] > Range (min … max): 3.953 s … 4.128 s 3 runs > > Summary > 'pks-connectivity-check-hide-refs' ran > 7.42 ± 0.16 times faster than 'main' And impressive, thanks! > Unfortunately, this change comes with a performance hit when refs are > not hidden. Executed in the same repository: > > Benchmark 1: main > Time (mean ± σ): 45.780 s ± 0.507 s [User: 46.908 s, System: 4.838 s] > Range (min … max): 45.453 s … 46.364 s 3 runs > > Benchmark 2: pks-connectivity-check-hide-refs > Time (mean ± σ): 49.886 s ± 0.282 s [User: 51.168 s, System: 5.015 s] > Range (min … max): 49.589 s … 50.149 s 3 runs > > Summary > 'main' ran > 1.09 ± 0.01 times faster than 'pks-connectivity-check-hide-refs' > > This is probably caused by the overhead of reachable tips being passed > in via git-rev-list(1)'s standard input, which seems to be slower than > reading the references from disk. > > It is debatable what to do about this. If this were only about improving > performance then it would be trivial to make the new logic depend on > whether or not `transfer.hideRefs` has been configured in the repo. But > as explained this is also about correctness, even though this can be > considered an edge case. Furthermore, this slowdown is really only > noticeable in outliers like the above repository with an unreasonable > amount of refs. The same benchmark in linux-stable.git with about > 4500 references shows no measurable difference: Do we have a test that would start failing if we changed the behavior? Perhaps such a test is peeking too much behind the curtain, but if it's easy come up with one I think it would be most welcome to have it alongside this. to have exposes > -static void write_head_info(void) > +static void write_head_info(struct oidset *announced_objects) > { > - static struct oidset seen = OIDSET_INIT; > - > - for_each_ref(show_ref_cb, &seen); > - for_each_alternate_ref(show_one_alternate_ref, &seen); > - oidset_clear(&seen); > + for_each_ref(show_ref_cb, announced_objects); > + for_each_alternate_ref(show_one_alternate_ref, announced_objects); > if (!sent_capabilities) > show_ref("capabilities^{}", null_oid()); Nit: The variable rename stands out slightly, i.e. s/&seen/announced_objects/ not s/&seen/seen/, especially as: > static void execute_commands(struct command *commands, > const char *unpacker_error, > struct shallow_info *si, > - const struct string_list *push_options) > + const struct string_list *push_options, > + struct oidset *announced_oids) Here we have the same variable, but now it's *_oids, not *objects. > + if (oidset_size(announced_oids) != 0) { Nit as before: The "!= 0" can go here. > @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > { > int advertise_refs = 0; > struct command *commands; > + struct oidset announced_oids = OIDSET_INIT; > struct oid_array shallow = OID_ARRAY_INIT; > struct oid_array ref = OID_ARRAY_INIT; > struct shallow_info si; > @@ -2524,7 +2536,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > } > > if (advertise_refs || !stateless_rpc) { > - write_head_info(); > + write_head_info(&announced_oids); > } > if (advertise_refs) > return 0; This introduces a memory leak to the function., We probably have other ones in code it calls, but from a quick eyeballing not in the function itself. Squashing in / combining it with this should do it, as it never returns non-zero (except for calling die()): diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 44bcea3a5b3..8d5c2fbef1c 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2527,7 +2527,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) write_head_info(); } if (advertise_refs) - return 0; + goto cleanup; packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE | @@ -2587,6 +2587,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) update_server_info(0); clear_shallow_info(&si); } +cleanup: if (use_sideband) packet_flush(1); oid_array_clear(&shallow); > @@ -2591,6 +2603,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > packet_flush(1); > oid_array_clear(&shallow); > oid_array_clear(&ref); > + oidset_clear(&announced_oids); > free((void *)push_cert_nonce); > return 0; > } We'll then properly reach this new oidset_clear()> The oid_array_clear() are all for variables we're populating after we're past htat "if (advertise_refs)". I think if you're re-rolling this sqashing 1/2 and 2/2 together would be an improvement. The 1/2 is tiny, and it's an API that's not used until this 1/2. I found myself going back & forth more than helped in reviewing this. Ggoing back a bit this: > +static const struct object_id *iterate_announced_oids(void *cb_data) > +{ > + struct oidset_iter *iter = cb_data; > + return oidset_iter_next(iter); > +} > + Is just used as (from 1/2): > + if (opt->reachable_oids_fn) { > + const struct object_id *reachable_oid; > + while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL) > + if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0) > + break; > + } After doing above: > + if (oidset_size(announced_oids) != 0) { > + oidset_iter_init(announced_oids, &announced_oids_iter); > + opt.reachable_oids_fn = iterate_announced_oids; > + opt.reachable_oids_data = &announced_oids_iter; > + } But I don't see the reason for the indirection, but maybe I'm missing something obvious. Why not just pass the oidset itself and have connected.c iterate through it, rather than going thorugh this callback / data indirection?
On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote: > This strategy has the major downside that it will not require any object > to be sent by the client that is reachable by any of the repositories' > references. While that sounds like it would be indeed what we are after > with the connectivity check, it is arguably not. The administrator that > manages the server-side Git repository may have configured certain refs > to be hidden during the reference advertisement via `transfer.hideRefs` > or `receivepack.hideRefs`. Whatever the reason, the result is that the > client shouldn't expect that any of those hidden references exists on > the remote side, and neither should they assume any of the pointed-to > objects to exist except if referenced by any visible reference. But > because we treat _all_ local refs as uninteresting in the connectivity > check, a client is free to send a packfile that references objects that > are only reachable via a hidden reference on the server-side, and we > will gladly accept it. You mention below that this is a correctness issue, but I am not sure that I agree. The existing behavior is a little strange, I agree, but your argument relies on an assumption that the history on hidden refs is not part of the reachable set, which is not the case. Any part of the repository that is reachable from _any_ reference, hidden or not, is reachable by definition. So it's perfectly fine to consider objects on hidden refs to be in the uninteresting set, because they are reachable. It's odd from the client's perspective, but I do not see a path to repository corruption with thee existing behavior. > Besides the stated fix to correctness this also provides a huge boost to > performance in the repository mentioned above. Pushing a new commit into > this repo with `transfer.hideRefs` set up to hide 6.8 million of 7 refs > as it is configured in Gitaly leads to an almost 7.5-fold speedup: Nice, here we expect a pretty good speed-up, and indeed... > Summary > 'pks-connectivity-check-hide-refs' ran > 7.42 ± 0.16 times faster than 'main' ...that's exactly what we get. Good. > @@ -1928,6 +1933,12 @@ static void execute_commands(struct command *commands, > opt.err_fd = err_fd; > opt.progress = err_fd && !quiet; > opt.env = tmp_objdir_env(tmp_objdir); > + if (oidset_size(announced_oids) != 0) { I'm nitpicking, but this would be preferable as "if (oidset_size(announced_oids))" without the "!= 0". > + oidset_iter_init(announced_oids, &announced_oids_iter); > + opt.reachable_oids_fn = iterate_announced_oids; > + opt.reachable_oids_data = &announced_oids_iter; > + } Why do we see a slowdown when there there aren't any hidden references? Or am I misunderstanding your patch message which instead means "we see a slow-down when there are no hidden references [since we still must store and enumerate all advertised references]"? If the latter, could we avoid invoking the new machinery altogether? In other words, shouldn't receive-pack only set the reachable_oids_fn() to enumerate advertised references only when the set of advertised references differs from the behavior of `--not --all`? > if (check_connected(iterate_receive_command_list, &data, &opt)) > set_connectivity_errors(commands, si); > > @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > { > int advertise_refs = 0; > struct command *commands; > + struct oidset announced_oids = OIDSET_INIT; This looks like trading one problem for another. In your above example, we now need to store 20 bytes of OIDs 6.8M times, or ~130 MiB. Not the end of the world, but it feels like an avoidable problem. Could we enumerate the references in a callback to for_each_ref() and only emit ones which aren't hidden? Storing these and then recalling them after the fact is worth avoiding. Thanks, Taylor
On Fri, Oct 28, 2022 at 05:01:58PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Fri, Oct 28 2022, Patrick Steinhardt wrote: [sinp] > > Unfortunately, this change comes with a performance hit when refs are > > not hidden. Executed in the same repository: > > > > Benchmark 1: main > > Time (mean ± σ): 45.780 s ± 0.507 s [User: 46.908 s, System: 4.838 s] > > Range (min … max): 45.453 s … 46.364 s 3 runs > > > > Benchmark 2: pks-connectivity-check-hide-refs > > Time (mean ± σ): 49.886 s ± 0.282 s [User: 51.168 s, System: 5.015 s] > > Range (min … max): 49.589 s … 50.149 s 3 runs > > > > Summary > > 'main' ran > > 1.09 ± 0.01 times faster than 'pks-connectivity-check-hide-refs' > > > > This is probably caused by the overhead of reachable tips being passed > > in via git-rev-list(1)'s standard input, which seems to be slower than > > reading the references from disk. > > > > It is debatable what to do about this. If this were only about improving > > performance then it would be trivial to make the new logic depend on > > whether or not `transfer.hideRefs` has been configured in the repo. But > > as explained this is also about correctness, even though this can be > > considered an edge case. Furthermore, this slowdown is really only > > noticeable in outliers like the above repository with an unreasonable > > amount of refs. The same benchmark in linux-stable.git with about > > 4500 references shows no measurable difference: > > Do we have a test that would start failing if we changed the behavior? > Perhaps such a test is peeking too much behind the curtain, but if it's > easy come up with one I think it would be most welcome to have it > alongside this. to have exposes We have tests that verify that we indeed detect missing objects in t5504. But what we're lacking is tests that verify that we stop walking at the boundary of preexisting objects, and I honestly wouldn't quite know how to do that as there is no functional difference, but really only a performance issue if we overwalked. > > -static void write_head_info(void) > > +static void write_head_info(struct oidset *announced_objects) > > { > > - static struct oidset seen = OIDSET_INIT; > > - > > - for_each_ref(show_ref_cb, &seen); > > - for_each_alternate_ref(show_one_alternate_ref, &seen); > > - oidset_clear(&seen); > > + for_each_ref(show_ref_cb, announced_objects); > > + for_each_alternate_ref(show_one_alternate_ref, announced_objects); > > if (!sent_capabilities) > > show_ref("capabilities^{}", null_oid()); > > Nit: The variable rename stands out slightly, > i.e. s/&seen/announced_objects/ not s/&seen/seen/, especially as: > > > static void execute_commands(struct command *commands, > > const char *unpacker_error, > > struct shallow_info *si, > > - const struct string_list *push_options) > > + const struct string_list *push_options, > > + struct oidset *announced_oids) > > Here we have the same variable, but now it's *_oids, not *objects. Hm. I think that `announced_oids` is easier to understand compared to `seen`, so I'd prefer to keep the rename. But I'll definitely make this consistent so we use `announced_oids` in both places. [snip] > > +static const struct object_id *iterate_announced_oids(void *cb_data) > > +{ > > + struct oidset_iter *iter = cb_data; > > + return oidset_iter_next(iter); > > +} > > + > > Is just used as (from 1/2): > > > + if (opt->reachable_oids_fn) { > > + const struct object_id *reachable_oid; > > + while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL) > > + if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0) > > + break; > > + } > > After doing above: > > > + if (oidset_size(announced_oids) != 0) { > > + oidset_iter_init(announced_oids, &announced_oids_iter); > > + opt.reachable_oids_fn = iterate_announced_oids; > > + opt.reachable_oids_data = &announced_oids_iter; > > + } > > But I don't see the reason for the indirection, but maybe I'm missing > something obvious. > > Why not just pass the oidset itself and have connected.c iterate through > it, rather than going thorugh this callback / data indirection? This is done to stay consistent with the way new tips are passed in via the `oid_iterate_fn`. I'm happy to change callers to just directly pass a `struct oidset *` though. Patrick
On Sun, Oct 30, 2022 at 03:09:04PM -0400, Taylor Blau wrote: > On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote: > > This strategy has the major downside that it will not require any object > > to be sent by the client that is reachable by any of the repositories' > > references. While that sounds like it would be indeed what we are after > > with the connectivity check, it is arguably not. The administrator that > > manages the server-side Git repository may have configured certain refs > > to be hidden during the reference advertisement via `transfer.hideRefs` > > or `receivepack.hideRefs`. Whatever the reason, the result is that the > > client shouldn't expect that any of those hidden references exists on > > the remote side, and neither should they assume any of the pointed-to > > objects to exist except if referenced by any visible reference. But > > because we treat _all_ local refs as uninteresting in the connectivity > > check, a client is free to send a packfile that references objects that > > are only reachable via a hidden reference on the server-side, and we > > will gladly accept it. > > You mention below that this is a correctness issue, but I am not sure > that I agree. > > The existing behavior is a little strange, I agree, but your argument > relies on an assumption that the history on hidden refs is not part of > the reachable set, which is not the case. Any part of the repository > that is reachable from _any_ reference, hidden or not, is reachable by > definition. > > So it's perfectly fine to consider objects on hidden refs to be in the > uninteresting set, because they are reachable. It's odd from the > client's perspective, but I do not see a path to repository corruption > with thee existing behavior. Indeed, I'm not trying to say that this can lead to repository corruption. If at all you can argue that this is more security-related. Suppose an object is not reachable from any public reference and that `allowAnySHA1InWant=false`. Then you could make these hidden objects reachable by sending a packfile with an object that references the hidden object. It naturally requires you to somehow know about the object ID, so I don't think this is a critical issue. But security-related or not, I think it is safe to say that any packfile sent by a client that does not contain objects required for the updated reference that the client cannot know to exist on the server-side must be generated by buggy code. [snip] > Why do we see a slowdown when there there aren't any hidden references? > Or am I misunderstanding your patch message which instead means "we see > a slow-down when there are no hidden references [since we still must > store and enumerate all advertised references]"? I have tried to dig down into the code of `revision.c` but ultimately returned empty-handed. I _think_ that this is because of the different paths we use when reading revisions from stdin as we have to resolve the revision to an OID first, which is more involved than taking the OIDs as returned by the reference backend. I have tried to short-circuit this logic in case the revision read from stdin is exactly `hash_algo->hexsz` long so that we try to parse it as an OID directly instead of trying to do any of the magic that is required to resolve a revision. But this only speed things up by a small margin. Another assumption was that this is overhead caused by using stdin instead of reading data from a file, but flame graphs didn't support this theory, either. > If the latter, could we avoid invoking the new machinery altogether? In > other words, shouldn't receive-pack only set the reachable_oids_fn() to > enumerate advertised references only when the set of advertised > references differs from the behavior of `--not --all`? Yeah, I was taking a "wait for feedback and see" stance on this. We can easily make the logic conditional on whether there are any hidden refs at all. > > if (check_connected(iterate_receive_command_list, &data, &opt)) > > set_connectivity_errors(commands, si); > > > > @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > > { > > int advertise_refs = 0; > > struct command *commands; > > + struct oidset announced_oids = OIDSET_INIT; > > This looks like trading one problem for another. In your above example, > we now need to store 20 bytes of OIDs 6.8M times, or ~130 MiB. Not the > end of the world, but it feels like an avoidable problem. We store these references in an `oidset` before this patch set already, but yes, the lifetime is longer now. But note that this set stores the announced objects, not the hidden ones. So we don't store 6.8m OIDs, but only the 250k announced ones. > Could we enumerate the references in a callback to for_each_ref() and > only emit ones which aren't hidden? Storing these and then recalling > them after the fact is worth avoiding. Sorry, I don't quite get what you're proposing. `for_each_ref()` already does exactly that: it stores every reference that is not hidden in the above `oidset`. This is the exact set of advertised references, which in my example repository would be about 250k. This information is used by git-receive-pack(1) to avoid announcing the same object twice, and now it's also used to inform the connectivity check to use these objects as the set of already-reachable objects. Patrick
On Mon, Oct 31 2022, Patrick Steinhardt wrote: > [[PGP Signed Part:Undecided]] > On Fri, Oct 28, 2022 at 05:01:58PM +0200, Ævar Arnfjörð Bjarmason wrote: >> On Fri, Oct 28 2022, Patrick Steinhardt wrote: > [sinp] >> > Unfortunately, this change comes with a performance hit when refs are >> > not hidden. Executed in the same repository: >> > >> > Benchmark 1: main >> > Time (mean ± σ): 45.780 s ± 0.507 s [User: 46.908 s, System: 4.838 s] >> > Range (min … max): 45.453 s … 46.364 s 3 runs >> > >> > Benchmark 2: pks-connectivity-check-hide-refs >> > Time (mean ± σ): 49.886 s ± 0.282 s [User: 51.168 s, System: 5.015 s] >> > Range (min … max): 49.589 s … 50.149 s 3 runs >> > >> > Summary >> > 'main' ran >> > 1.09 ± 0.01 times faster than 'pks-connectivity-check-hide-refs' >> > >> > This is probably caused by the overhead of reachable tips being passed >> > in via git-rev-list(1)'s standard input, which seems to be slower than >> > reading the references from disk. >> > >> > It is debatable what to do about this. If this were only about improving >> > performance then it would be trivial to make the new logic depend on >> > whether or not `transfer.hideRefs` has been configured in the repo. But >> > as explained this is also about correctness, even though this can be >> > considered an edge case. Furthermore, this slowdown is really only >> > noticeable in outliers like the above repository with an unreasonable >> > amount of refs. The same benchmark in linux-stable.git with about >> > 4500 references shows no measurable difference: >> >> Do we have a test that would start failing if we changed the behavior? >> Perhaps such a test is peeking too much behind the curtain, but if it's >> easy come up with one I think it would be most welcome to have it >> alongside this. to have exposes > > We have tests that verify that we indeed detect missing objects in > t5504. But what we're lacking is tests that verify that we stop walking > at the boundary of preexisting objects, and I honestly wouldn't quite > know how to do that as there is no functional difference, but really > only a performance issue if we overwalked. > >> > -static void write_head_info(void) >> > +static void write_head_info(struct oidset *announced_objects) >> > { >> > - static struct oidset seen = OIDSET_INIT; >> > - >> > - for_each_ref(show_ref_cb, &seen); >> > - for_each_alternate_ref(show_one_alternate_ref, &seen); >> > - oidset_clear(&seen); >> > + for_each_ref(show_ref_cb, announced_objects); >> > + for_each_alternate_ref(show_one_alternate_ref, announced_objects); >> > if (!sent_capabilities) >> > show_ref("capabilities^{}", null_oid()); >> >> Nit: The variable rename stands out slightly, >> i.e. s/&seen/announced_objects/ not s/&seen/seen/, especially as: >> >> > static void execute_commands(struct command *commands, >> > const char *unpacker_error, >> > struct shallow_info *si, >> > - const struct string_list *push_options) >> > + const struct string_list *push_options, >> > + struct oidset *announced_oids) >> >> Here we have the same variable, but now it's *_oids, not *objects. > > Hm. I think that `announced_oids` is easier to understand compared to > `seen`, so I'd prefer to keep the rename. But I'll definitely make this > consistent so we use `announced_oids` in both places. Sounds good, we'll need to look at the diff lines in either case (as we're converting it to a pointer), so changing the name while at it is fine... > [snip] >> > +static const struct object_id *iterate_announced_oids(void *cb_data) >> > +{ >> > + struct oidset_iter *iter = cb_data; >> > + return oidset_iter_next(iter); >> > +} >> > + >> >> Is just used as (from 1/2): >> >> > + if (opt->reachable_oids_fn) { >> > + const struct object_id *reachable_oid; >> > + while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL) >> > + if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0) >> > + break; >> > + } >> >> After doing above: >> >> > + if (oidset_size(announced_oids) != 0) { >> > + oidset_iter_init(announced_oids, &announced_oids_iter); >> > + opt.reachable_oids_fn = iterate_announced_oids; >> > + opt.reachable_oids_data = &announced_oids_iter; >> > + } >> >> But I don't see the reason for the indirection, but maybe I'm missing >> something obvious. >> >> Why not just pass the oidset itself and have connected.c iterate through >> it, rather than going thorugh this callback / data indirection? > > This is done to stay consistent with the way new tips are passed in via > the `oid_iterate_fn`. I'm happy to change callers to just directly pass > a `struct oidset *` though. *nod*, makes sense, no need to change it. Just wondering...
On Mon, Oct 31, 2022 at 03:45:47PM +0100, Patrick Steinhardt wrote: > On Sun, Oct 30, 2022 at 03:09:04PM -0400, Taylor Blau wrote: > > On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote: > > > This strategy has the major downside that it will not require any object > > > to be sent by the client that is reachable by any of the repositories' > > > references. While that sounds like it would be indeed what we are after > > > with the connectivity check, it is arguably not. The administrator that > > > manages the server-side Git repository may have configured certain refs > > > to be hidden during the reference advertisement via `transfer.hideRefs` > > > or `receivepack.hideRefs`. Whatever the reason, the result is that the > > > client shouldn't expect that any of those hidden references exists on > > > the remote side, and neither should they assume any of the pointed-to > > > objects to exist except if referenced by any visible reference. But > > > because we treat _all_ local refs as uninteresting in the connectivity > > > check, a client is free to send a packfile that references objects that > > > are only reachable via a hidden reference on the server-side, and we > > > will gladly accept it. > > > > You mention below that this is a correctness issue, but I am not sure > > that I agree. > > > > The existing behavior is a little strange, I agree, but your argument > > relies on an assumption that the history on hidden refs is not part of > > the reachable set, which is not the case. Any part of the repository > > that is reachable from _any_ reference, hidden or not, is reachable by > > definition. > > > > So it's perfectly fine to consider objects on hidden refs to be in the > > uninteresting set, because they are reachable. It's odd from the > > client's perspective, but I do not see a path to repository corruption > > with thee existing behavior. > > Indeed, I'm not trying to say that this can lead to repository > corruption. I definitely agree with that. I have thought about this on-and-off since you sent the topic, and I am pretty certain that there is no path to repository corruption with the existing behavior. It would be worth updating the commit message to make this clearer. > But security-related or not, I think it is safe to say that any packfile > sent by a client that does not contain objects required for the updated > reference that the client cannot know to exist on the server-side must > be generated by buggy code. Maybe, though I think it's fine to let clients send us smaller packfiles if they have some a-priori knowledge that the server has objects that it isn't advertising. And that can all happen without buggy code. So it's weird, but there isn't anything wrong with letting it happen. > [snip] > > Why do we see a slowdown when there there aren't any hidden references? > > Or am I misunderstanding your patch message which instead means "we see > > a slow-down when there are no hidden references [since we still must > > store and enumerate all advertised references]"? > > I have tried to dig down into the code of `revision.c` but ultimately > returned empty-handed. I _think_ that this is because of the different > paths we use when reading revisions from stdin as we have to resolve the > revision to an OID first, which is more involved than taking the OIDs as > returned by the reference backend. I have tried to short-circuit this > logic in case the revision read from stdin is exactly `hash_algo->hexsz` > long so that we try to parse it as an OID directly instead of trying to > do any of the magic that is required to resolve a revision. But this > only speed things up by a small margin. > > Another assumption was that this is overhead caused by using stdin > instead of reading data from a file, but flame graphs didn't support > this theory, either. > > > If the latter, could we avoid invoking the new machinery altogether? In > > other words, shouldn't receive-pack only set the reachable_oids_fn() to > > enumerate advertised references only when the set of advertised > > references differs from the behavior of `--not --all`? > > Yeah, I was taking a "wait for feedback and see" stance on this. We can > easily make the logic conditional on whether there are any hidden refs > at all. Yeah, I think that this would be preferable. I'm surprised that your data doesn't support the idea that the slowdown is caused by reading from stdin instead of parsing `--not --all`. I'd be curious to see what you have tried so far. I'm almost certain that forcing rev-list to chew through a bunch of data on stdin that is basically equivalent to saying `--not --all` is going to be the source of the slowdown. > > > if (check_connected(iterate_receive_command_list, &data, &opt)) > > > set_connectivity_errors(commands, si); > > > > > > @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > > > { > > > int advertise_refs = 0; > > > struct command *commands; > > > + struct oidset announced_oids = OIDSET_INIT; > > > > This looks like trading one problem for another. In your above example, > > we now need to store 20 bytes of OIDs 6.8M times, or ~130 MiB. Not the > > end of the world, but it feels like an avoidable problem. > > We store these references in an `oidset` before this patch set already, > but yes, the lifetime is longer now. But note that this set stores the > announced objects, not the hidden ones. So we don't store 6.8m OIDs, but > only the 250k announced ones. Hmm, OK. I wonder, could rev-list be given a `--refs=<namespace>` argument which is equal to the advertised references? Or something that implies "give me all of the references which aren't hidden?" If we call that maybe `--visible-refs=receive` (to imply the references *not* in receive.hideRefs), then our connectivity check would become: git rev-list --stdin --not --visible-refs=receive or something like that. Not having to extend the lifetime of these OIDs in an oidset would be worthwhile. Besides, having a new rev-list option is cool, too ;-). > > Could we enumerate the references in a callback to for_each_ref() and > > only emit ones which aren't hidden? Storing these and then recalling > > them after the fact is worth avoiding. > > Sorry, I don't quite get what you're proposing. `for_each_ref()` already > does exactly that: it stores every reference that is not hidden in the > above `oidset`. This is the exact set of advertised references, which in > my example repository would be about 250k. This information is used by > git-receive-pack(1) to avoid announcing the same object twice, and now > it's also used to inform the connectivity check to use these objects as > the set of already-reachable objects. Yes, ignore me ;-). Thanks, Taylor
On Mon, Oct 31, 2022 at 09:28:09PM -0400, Taylor Blau wrote: > On Mon, Oct 31, 2022 at 03:45:47PM +0100, Patrick Steinhardt wrote: > > On Sun, Oct 30, 2022 at 03:09:04PM -0400, Taylor Blau wrote: > > > On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote: > > > > This strategy has the major downside that it will not require any object > > > > to be sent by the client that is reachable by any of the repositories' > > > > references. While that sounds like it would be indeed what we are after > > > > with the connectivity check, it is arguably not. The administrator that > > > > manages the server-side Git repository may have configured certain refs > > > > to be hidden during the reference advertisement via `transfer.hideRefs` > > > > or `receivepack.hideRefs`. Whatever the reason, the result is that the > > > > client shouldn't expect that any of those hidden references exists on > > > > the remote side, and neither should they assume any of the pointed-to > > > > objects to exist except if referenced by any visible reference. But > > > > because we treat _all_ local refs as uninteresting in the connectivity > > > > check, a client is free to send a packfile that references objects that > > > > are only reachable via a hidden reference on the server-side, and we > > > > will gladly accept it. > > > > > > You mention below that this is a correctness issue, but I am not sure > > > that I agree. > > > > > > The existing behavior is a little strange, I agree, but your argument > > > relies on an assumption that the history on hidden refs is not part of > > > the reachable set, which is not the case. Any part of the repository > > > that is reachable from _any_ reference, hidden or not, is reachable by > > > definition. > > > > > > So it's perfectly fine to consider objects on hidden refs to be in the > > > uninteresting set, because they are reachable. It's odd from the > > > client's perspective, but I do not see a path to repository corruption > > > with thee existing behavior. > > > > Indeed, I'm not trying to say that this can lead to repository > > corruption. > > I definitely agree with that. I have thought about this on-and-off since > you sent the topic, and I am pretty certain that there is no path to > repository corruption with the existing behavior. It would be worth > updating the commit message to make this clearer. Fair enough, I can try to do that. > > But security-related or not, I think it is safe to say that any packfile > > sent by a client that does not contain objects required for the updated > > reference that the client cannot know to exist on the server-side must > > be generated by buggy code. > > Maybe, though I think it's fine to let clients send us smaller packfiles > if they have some a-priori knowledge that the server has objects that it > isn't advertising. And that can all happen without buggy code. So it's > weird, but there isn't anything wrong with letting it happen. Well, I don't see how to achieve both at the same time though: we can either limit the set of uninteresting tips to what we have announced to the client, or we allow clients to omit objects that have not been announced. These are mutually exclusive. So if we take the stance that it was fine to send packfiles that omit hidden objects and that this is something we want to continue to support then this patch series probably becomes moot. Doing the proposed optimization means that we also tighten the rules here. > > [snip] > > > Why do we see a slowdown when there there aren't any hidden references? > > > Or am I misunderstanding your patch message which instead means "we see > > > a slow-down when there are no hidden references [since we still must > > > store and enumerate all advertised references]"? > > > > I have tried to dig down into the code of `revision.c` but ultimately > > returned empty-handed. I _think_ that this is because of the different > > paths we use when reading revisions from stdin as we have to resolve the > > revision to an OID first, which is more involved than taking the OIDs as > > returned by the reference backend. I have tried to short-circuit this > > logic in case the revision read from stdin is exactly `hash_algo->hexsz` > > long so that we try to parse it as an OID directly instead of trying to > > do any of the magic that is required to resolve a revision. But this > > only speed things up by a small margin. > > > > Another assumption was that this is overhead caused by using stdin > > instead of reading data from a file, but flame graphs didn't support > > this theory, either. > > > > > If the latter, could we avoid invoking the new machinery altogether? In > > > other words, shouldn't receive-pack only set the reachable_oids_fn() to > > > enumerate advertised references only when the set of advertised > > > references differs from the behavior of `--not --all`? > > > > Yeah, I was taking a "wait for feedback and see" stance on this. We can > > easily make the logic conditional on whether there are any hidden refs > > at all. > > Yeah, I think that this would be preferable. I'm surprised that your > data doesn't support the idea that the slowdown is caused by reading > from stdin instead of parsing `--not --all`. I'd be curious to see what > you have tried so far. > > I'm almost certain that forcing rev-list to chew through a bunch of data > on stdin that is basically equivalent to saying `--not --all` is going > to be the source of the slowdown. I was basically benchmarking these two commands in the repository with 7m refs: # old-style connectivity check $ git commit-tree HEAD^{tree} -m commit -p HEAD >newtip $ git rev-list --objects --quiet --stdin --not --all <newtip # new-style connectivity check $ ( git for-each-ref --format='^%(objectname)' ; $ git commit-tree HEAD^{tree} -m commit -p HEAD ) >oldandnewtips $ git rev-list --objects --quiet --stdin <oldandnewtips I may have another look today and maybe share some flame graphs if I continue to not see anything obvious. > > > > if (check_connected(iterate_receive_command_list, &data, &opt)) > > > > set_connectivity_errors(commands, si); > > > > > > > > @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > > > > { > > > > int advertise_refs = 0; > > > > struct command *commands; > > > > + struct oidset announced_oids = OIDSET_INIT; > > > > > > This looks like trading one problem for another. In your above example, > > > we now need to store 20 bytes of OIDs 6.8M times, or ~130 MiB. Not the > > > end of the world, but it feels like an avoidable problem. > > > > We store these references in an `oidset` before this patch set already, > > but yes, the lifetime is longer now. But note that this set stores the > > announced objects, not the hidden ones. So we don't store 6.8m OIDs, but > > only the 250k announced ones. > > Hmm, OK. I wonder, could rev-list be given a `--refs=<namespace>` > argument which is equal to the advertised references? Or something that > implies "give me all of the references which aren't hidden?" > > If we call that maybe `--visible-refs=receive` (to imply the references > *not* in receive.hideRefs), then our connectivity check would become: > > git rev-list --stdin --not --visible-refs=receive > > or something like that. Not having to extend the lifetime of these OIDs > in an oidset would be worthwhile. Besides, having a new rev-list option > is cool, too ;-). That is indeed an interesting idea. It would likely both fix the issue of extended memory lifetime of the announced OIDs as well as fixing the slowdown caused by using `--stdin` for them. Patrick
On Mon, Oct 31, 2022 at 03:45:47PM +0100, Patrick Steinhardt wrote: > Indeed, I'm not trying to say that this can lead to repository > corruption. If at all you can argue that this is more security-related. > Suppose an object is not reachable from any public reference and that > `allowAnySHA1InWant=false`. Then you could make these hidden objects > reachable by sending a packfile with an object that references the > hidden object. It naturally requires you to somehow know about the > object ID, so I don't think this is a critical issue. I'd have to double check, but isn't this all moot with the v2 protocol anyway? I didn't think it even respected allowAnySHA1InWant. Even that aside, there are other tricks you can do. E.g., pushing up an object which claims to be a delta of X (though you have the trick then of figuring out how to reference it), or pushing up a test object then fetching it back while claiming to have X, treating the server as an oracle which may give you a delta against X. In short, I don't think it's worth considering unreachable objects in a repository to be considered secret. > > Why do we see a slowdown when there there aren't any hidden references? > > Or am I misunderstanding your patch message which instead means "we see > > a slow-down when there are no hidden references [since we still must > > store and enumerate all advertised references]"? > > I have tried to dig down into the code of `revision.c` but ultimately > returned empty-handed. I _think_ that this is because of the different > paths we use when reading revisions from stdin as we have to resolve the > revision to an OID first, which is more involved than taking the OIDs as > returned by the reference backend. I have tried to short-circuit this > logic in case the revision read from stdin is exactly `hash_algo->hexsz` > long so that we try to parse it as an OID directly instead of trying to > do any of the magic that is required to resolve a revision. But this > only speed things up by a small margin. > > Another assumption was that this is overhead caused by using stdin > instead of reading data from a file, but flame graphs didn't support > this theory, either. Certainly read_revisions_from_stdin() will allocate a bunch of extra data per item it reads (via add_rev_cmdline() and add_pending), which is going to be way more than parsing or stdin overhead. Much worse is that it will call get_reference(), which is very keen to actually open and parse the object (as you know, since you added commit-graph handling there). That might have gotten a bit better in v2.38.0 if you have any references to blobs (as we'd now skip the extra hash check). Of course the original "rev-list --not --all" would suffer from the same thing, so some of that may not explain any difference between the two. -Peff
On Tue, Nov 01, 2022 at 08:20:48AM +0100, Patrick Steinhardt wrote: > On Mon, Oct 31, 2022 at 09:28:09PM -0400, Taylor Blau wrote: > > On Mon, Oct 31, 2022 at 03:45:47PM +0100, Patrick Steinhardt wrote: > > > On Sun, Oct 30, 2022 at 03:09:04PM -0400, Taylor Blau wrote: > > > > On Fri, Oct 28, 2022 at 04:42:27PM +0200, Patrick Steinhardt wrote: > > > > > This strategy has the major downside that it will not require any object > > > > > to be sent by the client that is reachable by any of the repositories' > > > > > references. While that sounds like it would be indeed what we are after > > > > > with the connectivity check, it is arguably not. The administrator that > > > > > manages the server-side Git repository may have configured certain refs > > > > > to be hidden during the reference advertisement via `transfer.hideRefs` > > > > > or `receivepack.hideRefs`. Whatever the reason, the result is that the > > > > > client shouldn't expect that any of those hidden references exists on > > > > > the remote side, and neither should they assume any of the pointed-to > > > > > objects to exist except if referenced by any visible reference. But > > > > > because we treat _all_ local refs as uninteresting in the connectivity > > > > > check, a client is free to send a packfile that references objects that > > > > > are only reachable via a hidden reference on the server-side, and we > > > > > will gladly accept it. > > > > > > > > You mention below that this is a correctness issue, but I am not sure > > > > that I agree. > > > > > > > > The existing behavior is a little strange, I agree, but your argument > > > > relies on an assumption that the history on hidden refs is not part of > > > > the reachable set, which is not the case. Any part of the repository > > > > that is reachable from _any_ reference, hidden or not, is reachable by > > > > definition. > > > > > > > > So it's perfectly fine to consider objects on hidden refs to be in the > > > > uninteresting set, because they are reachable. It's odd from the > > > > client's perspective, but I do not see a path to repository corruption > > > > with thee existing behavior. > > > > > > Indeed, I'm not trying to say that this can lead to repository > > > corruption. > > > > I definitely agree with that. I have thought about this on-and-off since > > you sent the topic, and I am pretty certain that there is no path to > > repository corruption with the existing behavior. It would be worth > > updating the commit message to make this clearer. > > Fair enough, I can try to do that. > > > > But security-related or not, I think it is safe to say that any packfile > > > sent by a client that does not contain objects required for the updated > > > reference that the client cannot know to exist on the server-side must > > > be generated by buggy code. > > > > Maybe, though I think it's fine to let clients send us smaller packfiles > > if they have some a-priori knowledge that the server has objects that it > > isn't advertising. And that can all happen without buggy code. So it's > > weird, but there isn't anything wrong with letting it happen. > > Well, I don't see how to achieve both at the same time though: we can > either limit the set of uninteresting tips to what we have announced to > the client, or we allow clients to omit objects that have not been > announced. These are mutually exclusive. > > So if we take the stance that it was fine to send packfiles that omit > hidden objects and that this is something we want to continue to support > then this patch series probably becomes moot. Doing the proposed > optimization means that we also tighten the rules here. I'm wrong and you're right: we can do the optimization to limit the refs we use but still let clients send objects that are hidden. I didn't take into account that this is merely an optimization that we stop walking at reachable tips. I'll reword the commit message when having another go and will likely do something along the lines of your proposed new `--visible-refs` option in v2 of this series. Patrick
On Tue, Nov 01, 2022 at 12:53:42PM +0100, Patrick Steinhardt wrote: > > > Maybe, though I think it's fine to let clients send us smaller packfiles > > > if they have some a-priori knowledge that the server has objects that it > > > isn't advertising. And that can all happen without buggy code. So it's > > > weird, but there isn't anything wrong with letting it happen. > > > > Well, I don't see how to achieve both at the same time though: we can > > either limit the set of uninteresting tips to what we have announced to > > the client, or we allow clients to omit objects that have not been > > announced. These are mutually exclusive. > > > > So if we take the stance that it was fine to send packfiles that omit > > hidden objects and that this is something we want to continue to support > > then this patch series probably becomes moot. Doing the proposed > > optimization means that we also tighten the rules here. > > I'm wrong and you're right: we can do the optimization to limit the refs > we use but still let clients send objects that are hidden. I didn't take > into account that this is merely an optimization that we stop walking at > reachable tips. I'll reword the commit message when having another go > and will likely do something along the lines of your proposed new > `--visible-refs` option in v2 of this series. I wasn't necessarily advocating for a behavior change in this series, more pointing out that the situation you said can only happen with buggy code doesn't actually require a bug in practice. Thanks, Taylor
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 44bcea3a5b..50794539c6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -326,13 +326,10 @@ static void show_one_alternate_ref(const struct object_id *oid, show_ref(".have", oid); } -static void write_head_info(void) +static void write_head_info(struct oidset *announced_objects) { - static struct oidset seen = OIDSET_INIT; - - for_each_ref(show_ref_cb, &seen); - for_each_alternate_ref(show_one_alternate_ref, &seen); - oidset_clear(&seen); + for_each_ref(show_ref_cb, announced_objects); + for_each_alternate_ref(show_one_alternate_ref, announced_objects); if (!sent_capabilities) show_ref("capabilities^{}", null_oid()); @@ -1896,12 +1893,20 @@ static void execute_commands_atomic(struct command *commands, strbuf_release(&err); } +static const struct object_id *iterate_announced_oids(void *cb_data) +{ + struct oidset_iter *iter = cb_data; + return oidset_iter_next(iter); +} + static void execute_commands(struct command *commands, const char *unpacker_error, struct shallow_info *si, - const struct string_list *push_options) + const struct string_list *push_options, + struct oidset *announced_oids) { struct check_connected_options opt = CHECK_CONNECTED_INIT; + struct oidset_iter announced_oids_iter; struct command *cmd; struct iterate_data data; struct async muxer; @@ -1928,6 +1933,12 @@ static void execute_commands(struct command *commands, opt.err_fd = err_fd; opt.progress = err_fd && !quiet; opt.env = tmp_objdir_env(tmp_objdir); + if (oidset_size(announced_oids) != 0) { + oidset_iter_init(announced_oids, &announced_oids_iter); + opt.reachable_oids_fn = iterate_announced_oids; + opt.reachable_oids_data = &announced_oids_iter; + } + if (check_connected(iterate_receive_command_list, &data, &opt)) set_connectivity_errors(commands, si); @@ -2462,6 +2473,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) { int advertise_refs = 0; struct command *commands; + struct oidset announced_oids = OIDSET_INIT; struct oid_array shallow = OID_ARRAY_INIT; struct oid_array ref = OID_ARRAY_INIT; struct shallow_info si; @@ -2524,7 +2536,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) } if (advertise_refs || !stateless_rpc) { - write_head_info(); + write_head_info(&announced_oids); } if (advertise_refs) return 0; @@ -2554,7 +2566,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) } use_keepalive = KEEPALIVE_ALWAYS; execute_commands(commands, unpack_status, &si, - &push_options); + &push_options, &announced_oids); if (pack_lockfile) unlink_or_warn(pack_lockfile); sigchain_push(SIGPIPE, SIG_IGN); @@ -2591,6 +2603,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) packet_flush(1); oid_array_clear(&shallow); oid_array_clear(&ref); + oidset_clear(&announced_oids); free((void *)push_cert_nonce); return 0; }
When serving a push, git-receive-pack(1) needs to verify that the packfile sent by the client contains all objects that are required by the updated references. This connectivity check works by marking all preexisting references as uninteresting and using the new reference tips as starting point for a graph walk. This strategy has the major downside that it will not require any object to be sent by the client that is reachable by any of the repositories' references. While that sounds like it would be indeed what we are after with the connectivity check, it is arguably not. The administrator that manages the server-side Git repository may have configured certain refs to be hidden during the reference advertisement via `transfer.hideRefs` or `receivepack.hideRefs`. Whatever the reason, the result is that the client shouldn't expect that any of those hidden references exists on the remote side, and neither should they assume any of the pointed-to objects to exist except if referenced by any visible reference. But because we treat _all_ local refs as uninteresting in the connectivity check, a client is free to send a packfile that references objects that are only reachable via a hidden reference on the server-side, and we will gladly accept it. Besides the correctness issue there is also a performance issue. Git forges tend to do internal bookkeeping to keep alive sets of objects for internal use or make them easy to find via certain references. These references are typically hidden away from the user so that they are neither advertised nor writeable. At GitLab, we have one particular repository that contains a total of 7 million references, of which 6.8 million are indeed internal references. With the current connectivity check we are forced to load all these references in order to mark them as uninteresting, and this alone takes around 15 seconds to compute. We can fix both of these issues by changing the logic for stateful invocations of git-receive-pack(1) where the reference advertisement and packfile negotiation are served by the same process. Instead of marking all preexisting references as unreachable, we will only mark those that we have announced to the client. Besides the stated fix to correctness this also provides a huge boost to performance in the repository mentioned above. Pushing a new commit into this repo with `transfer.hideRefs` set up to hide 6.8 million of 7 refs as it is configured in Gitaly leads to an almost 7.5-fold speedup: Benchmark 1: main Time (mean ± σ): 29.902 s ± 0.105 s [User: 29.176 s, System: 1.052 s] Range (min … max): 29.781 s … 29.969 s 3 runs Benchmark 2: pks-connectivity-check-hide-refs Time (mean ± σ): 4.033 s ± 0.088 s [User: 4.071 s, System: 0.374 s] Range (min … max): 3.953 s … 4.128 s 3 runs Summary 'pks-connectivity-check-hide-refs' ran 7.42 ± 0.16 times faster than 'main' Unfortunately, this change comes with a performance hit when refs are not hidden. Executed in the same repository: Benchmark 1: main Time (mean ± σ): 45.780 s ± 0.507 s [User: 46.908 s, System: 4.838 s] Range (min … max): 45.453 s … 46.364 s 3 runs Benchmark 2: pks-connectivity-check-hide-refs Time (mean ± σ): 49.886 s ± 0.282 s [User: 51.168 s, System: 5.015 s] Range (min … max): 49.589 s … 50.149 s 3 runs Summary 'main' ran 1.09 ± 0.01 times faster than 'pks-connectivity-check-hide-refs' This is probably caused by the overhead of reachable tips being passed in via git-rev-list(1)'s standard input, which seems to be slower than reading the references from disk. It is debatable what to do about this. If this were only about improving performance then it would be trivial to make the new logic depend on whether or not `transfer.hideRefs` has been configured in the repo. But as explained this is also about correctness, even though this can be considered an edge case. Furthermore, this slowdown is really only noticeable in outliers like the above repository with an unreasonable amount of refs. The same benchmark in linux-stable.git with about 4500 references shows no measurable difference: Benchmark 1: main Time (mean ± σ): 375.4 ms ± 25.4 ms [User: 312.2 ms, System: 155.7 ms] Range (min … max): 324.2 ms … 492.9 ms 50 runs Benchmark 2: pks-connectivity-check-hide-refs Time (mean ± σ): 374.9 ms ± 36.9 ms [User: 311.6 ms, System: 158.2 ms] Range (min … max): 319.2 ms … 583.1 ms 50 runs Summary 'pks-connectivity-check-hide-refs' ran 1.00 ± 0.12 times faster than 'main' Let's keep this as-is for the time being and accept the performance hit. It is arguably extremely noticeable to a user if a push now performs 7.5 times faster than before, but a lot less so in case an already-slow push becomes about 10% slower. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/receive-pack.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)