Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Justin Tobler <jltobler@gmail.com>]

On 26/10/28 03:18PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> 
> Since 722ff7f876 (receive-pack: quarantine objects until pre-receive
> accepts, 2016-10-03), receive-pack writes incoming objects to a
> temporary object directory. Child processes see it as their primary
> object directory, and tmp-objdir passes the repository's own object
> directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.
> 
> The connectivity check passes --alternate-refs to rev-list since
> 39b44ba771 (check_everything_connected: assume alternate ref tips are
> valid, 2019-07-01). Inside the quarantine, the repository's own object
> directory is one of the alternates, so --alternate-refs runs
> for-each-ref on the repository itself and lists all of its refs.

That is quite interesting. So IIUC, because the "real" ODB is an
alternate, all the references from the main repository are marked as
uninteresting even though they have already been processed. For the
connectivity check performed by git-receive-pack(1), there isn't any
reason to consider "alternate refs" from the "real" ODB that is
configured as an alternate.

> That includes hidden refs. bcec6780b2 (receive-pack: only use visible
> refs for connectivity check, 2022-11-17) added --exclude-hidden=receive
> to the check so that repositories with many hidden refs do not have to
> load all of them on each push. The hidden refs are excluded from --all,
> but they all come back through --alternate-refs.

So we effectively consider all the references a second time, but always
all of them including the previously excluded hidden refs. Yikes.

> Skip an alternate whose repository is our own when listing alternate
> refs. This is done by comparing paths, which is a hack: it is mainly
> here to measure the cost of the bug, and a proper fix would have the
> quarantine tell its children which directory is ours. It is safe,
> though: alternate refs only serve as negative tips that are assumed to
> be complete, so dropping some can only make the traversal walk further,
> never make it accept a missing object.

This indeed sounds like a bit of a hack. To me the underlying issue here
is that downstream git processes don't really have any concept of an ODB
transaction. We rely on the git alternates mechanism and overriding the
primary object directory to get the unaware child processes to behave
how we want. This typically means setting the quarantine directory as
the main object directory and the "real" object directory as an
alternate via environment variables. That way any writes that are
performed by a child process are written to the quarantine.

For the connectivity check though, we don't really expect
git-rev-list(1) to write any objects. Consequently, I don't think we
neccessary need to set the quarantine directory as the primary object
directory here. That would avoid git-rev-list(1) processing the "real"
object directory references a second time.

[snip]
>     I also tried resolving the object directories that --alternate-refs uses
>     before the quarantine is created. That got a bit messy: it needed a new
>     internal rev-list option, and receive-pack had to carry the list of
>     directories through to each of its connectivity checks. And it only
>     fixes the check itself; other processes running inside the quarantine,
>     like pre-receive hooks, would still see the repository as its own
>     alternate.

For the connectivity check performed via git-rev-list(1) it certainly
matters, but I do wonder though for other processes if this really
changes anything for them in practice. It could potentially be nice if
there was a mechanism to more explicitly propagate a pending transaction
accross process boundaries, but I'm not sure there are many usecases for
that at present.

>     So the question is really where this should be fixed: is there a better
>     way for processes running inside the quarantine to tell the repository's
>     own object directory apart from real alternates?

Building off of my previous thoughts, maybe it would make sense to make
the connectivity check a more explicit part of the ODB transaction
interface? Maybe for the "files" backend we could introduce a
`odb_transaction_files_check_connected()` callback that configures
git-rev-list(1) with the appropriate environment to validate the
quarantined objects and not cause this bug? Happy to explore this path
further if it seems reasonable.

-Justin