From: Kristofer Karlsson <krka@spotify.com>
Date: Wed, 04 Nov 2026 10:25:38 GMT
Subject: Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine
Message-ID: <CAL71e4OreARF+dbCU3S8-_umrFEptPLPUiPDH3SYBx6OYjVpfg@mail.gmail.com>
In-Reply-To: <20261104024806.GA1492379@coredump.intra.peff.net>

On Wed, 4 Nov 2026 at 03:48, Jeff King <peff@peff.net> wrote:
>
> I.e., I'd expect the code part of the patch to just be:
>
> diff --git a/odb.c b/odb.c
> index 2c93a9490c..f9ab5c8e7f 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
>                                   void *payload)
>  {
>         struct strbuf path = STRBUF_INIT;
> +       struct strbuf repo = STRBUF_INIT;
>         size_t base_len;
>         struct alternate_refs_data *cb = payload;
>
> @@ -198,10 +199,16 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
>                 goto out;
>         strbuf_setlen(&path, base_len);
>
> +       /* XXX could realpath once in caller as a mild optimization */
> +       if (strbuf_realpath(&repo, cb->repo->commondir, 0) &&
> +           !strcmp(path.buf, repo.buf))
> +               goto out;
> +
>         read_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);
>
>  out:
>         strbuf_release(&path);
> +       strbuf_release(&repo);
>         return 0;
>  }
>
>
> and keep it totally local to the path hackery in the alternate-refs
> path.
>
> Or am I missing something that requires more complication?

No, I think you are right.  The quarantine parent only matters when
the object directory is not under the commondir, which is rare.  Your
version is essentially my v1 without the "this is a hack" framing
(and also cleaner).

> > This is safe: 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.
>
> That's how _we_ use them in the internal callers, but it's a rev-list
> option. So we can't say how they are used elsewhere. That said, I think
> this is a totally reasonable tightening of the definition for general
> use.

Good point.  I will reword that part of the commit message so that it
does not rely on how the connectivity check uses them.  It also seems
in line with what we already do when loading alternates, where
listing the object directory itself is ignored as a common mistake;
inside the quarantine, that check just compares against the
quarantine instead of our own object directory.

Justin suggested fixing it at the transaction layer instead, so I will
wait to see where that discussion lands before sending a v3.

It looks like the remaining options are:
  1. Go with your proposed version.
  2. Do nothing until the long term fix.
  3. Just add a regression test for now.

Thanks,
Kristofer