Re: [PATCH 2/7] setup: introduce object storage extension

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 19, 2026 at 02:29:20PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb/source-files.c b/odb/source-files.c
> > index 455f409db0..4a7cbbfd9b 100644
> > --- a/odb/source-files.c
> > +++ b/odb/source-files.c
> > @@ -1245,6 +1246,12 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
> >  		object_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));
> >  		alternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
> >  	}
> > +	if (!object_dir && payload) {
> > +		if (is_absolute_path(payload))
> > +			object_dir = xstrdup(payload);
> > +		else
> > +			object_dir = xstrfmt("%s/%s", odb->repo->commondir, payload);
> > +	}
> 
> On the reference side, we use `strbuf_realpath()` to convert the
> relative paths to absolute paths, should we be doing that here too?

Overall I'd love for the "files" source to only ever use absolute paths,
but historically that wasn't the case. We sometimes expose those paths
to users, and changing the "files" source thus breaks a bunch of tests.

So I'm retaining that behaviour for now, also with the extension. It may
be something to explore going forward though.

> > diff --git a/odb/source.c b/odb/source.c
> > index b25ef14df8..10abe33a26 100644
> > --- a/odb/source.c
> > +++ b/odb/source.c
> > @@ -1,4 +1,5 @@
> >  #include "git-compat-util.h"
> > +#include "gettext.h"
> >  #include "object-file.h"
> >  #include "odb/source-files.h"
> >  #include "odb/source.h"
> 
> Do we need this import? This commit seems to compile without it.

Yeah, we add some translatable strings to `odb_source_new()`. The header
is also implicitly included, but as Junio pointed out we should include
headers explicitly regardless when we require their declarations.

> > diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> > index f1d227f271..c7c464d3a2 100755
> > --- a/t/t0001-init.sh
> > +++ b/t/t0001-init.sh
> > @@ -839,6 +839,36 @@ test_expect_success 'init with --ref-storage-format=garbage' '
> >  	test_cmp expect err
> >  '
> >
> > +test_expect_success DEFAULT_REPO_FORMAT 'extensions.objectStorage is not allowed with repo version 0' '
> > +	test_when_finished "rm -rf objstorage" &&
> > +	git init objstorage &&
> > +	git -C objstorage config set extensions.objectStorage files &&
> > +	test_must_fail git -C objstorage rev-parse 2>err &&
> > +	test_grep "repo version is 0, but v1-only extension found" err
> > +'
> > +
> > +test_expect_success DEFAULT_REPO_FORMAT 'extensions.objectStorage with files backend' '
> > +	test_when_finished "rm -rf objstorage" &&
> > +	git init objstorage &&
> > +	git -C objstorage config set core.repositoryformatversion 1 &&
> 
> If we're doing this, do we still need DEFAULT_REPO_FORMAT?

We could indeed get away with not having the prerequisite right now. But
once we have a second backend we'd need this, because in that case we'd
potentially initialize the repository with a different backend here. And
then just switching over to the "files" backend would probably not
result in anything good.

So this is more of a protective measure for the future.

> > +	git -C objstorage config set extensions.objectStorage garbage &&
> > +	test_must_fail git -C objstorage rev-parse 2>err &&
> > +	test_grep "invalid value for ${SQ}extensions.objectstorage${SQ}: ${SQ}garbage${SQ}" err &&
> > +
> > +	git config --file objstorage/.git/config extensions.objectStorage garbage://payload &&
> > +	test_must_fail git -C objstorage rev-parse 2>err &&
> > +	test_grep "invalid value for ${SQ}extensions.objectstorage${SQ}: ${SQ}garbage://payload${SQ}" err
> > +'
> > +
> 
> We should add  tests for the files ODB with {valid, invalid} payload. We
> should probably also test absolute and relative paths in the valid flow.

I add tests for both absolute and relative paths in a later commit. Here
I'm basically just verifying whether the object storage extension even
kicks in at all, without yet verifying its semantics properly.

Patrick