Re: [PATCH v2 1/8] builtin/clone: defer setup of the object database

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 7, 2026, 07:23 UTC
Message-ID: <ap5mafyJrnXCTw_L@pks.im>
In-Reply-To: <ap2URrS4i-nHV5cB@denethor>

On Sun, Sep 06, 2026 at 11:42:48AM -0500, Justin Tobler wrote:

Show 25 quoted lines

> On 26/08/31 12:02PM, Patrick Steinhardt wrote:
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 5b25cca510..0a67492ebd 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1184,11 +1184,14 @@ int cmd_clone(int argc,
> >  	 * database. We do not yet know about the object format of the
> >  	 * repository, and reference backends may persist that information into
> >  	 * their on-disk data structures.
> > +	 *
> > +	 * Furthermore, we skip initializing the object database so that we can
> > +	 * first resolve potential alternates before creating it.
> >  	 */
> >  	init_db(the_repository, git_dir, real_git_dir, work_tree, option_template,
> >  		GIT_HASH_UNKNOWN, ref_storage_format, NULL,
> >  		do_not_override_repo_unix_permissions,
> > -		INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
> > +		INIT_DB_QUIET | INIT_DB_SKIP_REFDB | INIT_DB_SKIP_ODB);
> 
> Ok, now we skip the initializing the ODB during init_db in favor of
> delaying it to after we have the required config info. This makes sense
> to me, but IMO the `init_db()` interface has grown quite awkward with
> these "skip" flags. It appears that there are only two callers of
> `init_db()` which makes me wonder if it would be simpler to just require
> them to explicitly set up the ref DB and ODB.

We can, but there are some nuances here that make this a bit more complicated. Most importantly, we'd start printing the message that the repository was (re)initialized _before_ we create the actual object and reference databases.

But thinking about this a bit more we can solve this, and we can even get rid of the flags completely here:

  - git-clone(1) always passes the QUIET flag, so we really only want to
    print the message for git-init(1) anyway. So we can lift the logic
    out of `init_db()` and then drop the flag.

  - Same for `EXIST_OK`, we never allow preexisting repositories when
    performing a clone.

The name `init_db()` would become very misleading in that case though, so we should probably rename it to e.g. `create_repository()`. But overall the change makes sense, as it moves the command-specific logic to the commands themselves instead of making use of flags to control it. I like it.

Show 22 quoted lines

> > @@ -1311,9 +1314,6 @@ int cmd_clone(int argc,
> >  		strbuf_reset(&key);
> >  	}
> >  
> > -	if (option_required_reference.nr || option_optional_reference.nr)
> > -		setup_reference();
> > -
> >  	remote = remote_get_early(remote_name);
> >  
> >  	if (!option_rev)
> > @@ -1342,6 +1342,10 @@ int cmd_clone(int argc,
> >  	if (option_local > 0 && !is_local)
> >  		warning(_("--local is ignored"));
> >  
> > +	create_object_database(the_repository);
> 
> We now explicitly create the object database here.
> 
> > +	if (option_required_reference.nr || option_optional_reference.nr)
> > +		setup_reference();
> 
> Any reason the reference setup is also further deferred here?

You might think that this has something to do with refs ("refs/*"), but that's not the case. This setup here sets up alternates, and we can only set those up after we have created the object database. I'll note this in the commit message as it's quite non-obvious.

Thanks!

Patrick