Re: [PATCH 2/2] receive-pack: use advertised reference tips to inform connectivity check

> 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.

> [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.