From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 03 Nov 2026 11:00:47 GMT
Subject: Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine
Message-ID: <aum-34I8KZvTfDRT@pks.im>
In-Reply-To: <pull.2285.v2.git.1793619933300.gitgitgadget@gmail.com>

On Mon, Nov 02, 2026 at 11:45:33AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 5b4277fbfd..223af45dd9 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -348,6 +351,45 @@ static int read_alternates(const char *object_dir, struct strvec *out)
>  	return 0;
>  }
>  
> +/*
> + * Our own object directory is <commondir>/objects, or the parent of
> + * GIT_QUARANTINE_PATH, as an object quarantine is created directly

Or it may be derived from `files://<payload>` with the object storage
format extension.

> + * inside it (see tmp_objdir_create()). Anything else, including a path
> + * that cannot be resolved, is borrowed.
> + */
> +static bool is_borrowed_object_dir(struct odb_source_files *files,
> +				   const char *path)
> +{
> +	struct strbuf real = STRBUF_INIT;
> +	struct strbuf objdir = STRBUF_INIT;
> +	struct strbuf self = STRBUF_INIT;
> +	bool ret = true;
> +
> +	if (!strbuf_realpath(&real, path, 0))
> +		goto out;
> +
> +	strbuf_addf(&objdir, "%s/objects", files->base.odb->repo->commondir);
> +	if (strbuf_realpath(&self, objdir.buf, 0) &&
> +	    !fspathcmp(real.buf, self.buf)) {
> +		ret = false;
> +		goto out;
> +	}

So this here isn't fully correct as it does not account for all cases.
We'd have to read the object storage format extension to figure out
where the primary object directory lives.

> @@ -1256,15 +1299,19 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
>  	struct odb_source_files *files;
>  	char *object_dir = NULL;
>  	char *alternates = NULL;
> +	char *quarantine_path = NULL;
>  
>  	if (flags & ODB_NEW_HONOR_ENV) {
>  		object_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));
>  		alternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
> +		quarantine_path =
> +			xstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));
>  	}
>  	if (!object_dir)
>  		object_dir = xstrfmt("%s/objects", odb->repo->commondir);
>  
>  	CALLOC_ARRAY(files, 1);
> +	files->quarantine_path = quarantine_path;
>  	odb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);
>  
>  	hashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);

This makes me wonder whether the better fix would be to stop modifying
the primary object directory and instead we simply append the quarantine
directory to alternates. The presence of `GIT_QUARANTINE_ENVIRONMENT`
could then serve as an indicator that the child process is not allowed
to write objects.

That's a much bigger change in semantics though, and the risk of causing
a regression would be quite a bit higher. But the upside is that we
wouldn't have to juggle alternates anymore, and your case with alternate
refs would "just work".

Patrick