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
