Re: [PATCH 1/7] setup: extract function to create the refdb

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 7, 2023, 07:22 UTC
Message-ID: <ZXFy0_T1AZLh058g@tanuki>
In-Reply-To: <CAOLa=ZSZztJUF9nmSzGdOW0oWBRUp2sw8QyuZO_q06cNymad3Q@mail.gmail.com>

On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:

Show 14 quoted lines

> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> > +static void create_reference_database(const char *initial_branch, int quiet)
> > +{
> > +       struct strbuf err = STRBUF_INIT;
> > +       int reinit = is_reinit();
> > +
> > +       /*
> > +        * We need to create a "refs" dir in any case so that older
> > +        * versions of git can tell that this is a repository.
> > +        */
> 
> How does this work though, even if an earlier version of git can tell
> that this is a repository, it still won't be able to read the reftable
> backend. In that sense, what do we achieve here?

This is a good question, and there is related ongoing discussion about this topic in the thread starting at [1]. There are a few benefits to letting clients discover such repos even if they don't understand the new reference backend format:

  - They know to stop walking up the parent-directory chain. Otherwise a
    client might end up detecting a Git repository in the parent dir.

  - The user gets a proper error message why the repository cannot be
    accessed. Instead of failing to detect the repository altogether we
    instead say that we don't understand the "extensions.refFormat"
    extension.

Maybe there are other cases I can't think of right now.

Show 6 quoted lines

> > +       safe_create_dir(git_path("refs"), 1);
> > +       adjust_shared_perm(git_path("refs"));
> > +
> 
> Not related to your commit per se, but we ignore the return value
> here, shouldn't we die in this case?

While the end result wouldn't be quite what the user asks for, the only negative consequence is that the repository is inaccessible to others. I think this failure mode is comparatively benign -- if it were the other way round and we'd over-share the repository it would more severe.

So while I don't think that dying makes much sense here, I could certainly see us adding a warning so that the user at least knows that something went wrong. I'd rather want to keep this out of the current patch series, but could certainly see such a warning added in a follow up patch series.

Patrick

[1]: <ZWcOvjGPVS_CMUAk@tanuki>