From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 10 Sep 2026 11:10:31 GMT
Subject: Re: [PATCH v4 7/9] odb/source: support writing alternates when creating the database
Message-ID: <CAOLa=ZQaPstiQmXm9=TyWPUxL6X2=Lcqeg6y2XeXzSJDpq-GBA@mail.gmail.com>
In-Reply-To: <20260909-pks-odb-write-alternates-at-creation-time-v4-7-d8a78ffc32e4@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> Add the ability to write alternates when creating the object database.
> This change allows us to remove the `write_alternates()` callback in a
> subsequent patch.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-files.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  odb/source.h       | 17 +++++++++---
>  setup.c            |  4 ++-
>  3 files changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/odb/source-files.c b/odb/source-files.c
> index b7b3a297bb..8fe65d91f8 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -18,6 +18,7 @@
>  #include "run-command.h"
>  #include "strbuf.h"
>  #include "string-list.h"
> +#include "strmap.h"
>  #include "strvec.h"
>  #include "tree.h"
>  #include "write-or-die.h"
> @@ -51,9 +52,14 @@ static void odb_source_files_close(struct odb_source *source)
>  	odb_source_close(&files->packed->base);
>  }
>
> -static int odb_source_files_create_on_disk(struct odb_source *source)
> +static int odb_source_files_create_on_disk(struct odb_source *source,
> +					   const struct odb_create_on_disk_options *opts)
>  {
> +	struct lock_file alternates_lock = LOCK_INIT;
>  	struct strbuf path = STRBUF_INIT;
> +	struct strset seen = STRSET_INIT;
> +	struct strbuf line = STRBUF_INIT;
> +	int ret;
>
>  	safe_create_dir(source->odb->repo, source->path, 1);
>
> @@ -64,8 +70,74 @@ static int odb_source_files_create_on_disk(struct odb_source *source)
>  	strbuf_addf(&path, "%s/info", source->path);
>  	safe_create_dir(source->odb->repo, path.buf, 1);
>
> +	if (opts->alternates && opts->alternates->nr) {
> +		FILE *alternates, *orig;
> +

So this is similar to what we already do in
`odb_source_files_write_alternate()`.

> +		strbuf_reset(&path);
> +		strbuf_addf(&path, "%s/info/alternates", source->path);
> +
> +		repo_hold_lock_file_for_update(source->odb->repo, &alternates_lock,
> +					       path.buf, LOCK_DIE_ON_ERROR);
> +
> +		alternates = fdopen_lock_file(&alternates_lock, "w");
> +		if (!alternates) {
> +			ret = error_errno(_("unable to fdopen alternates lockfile"));
> +			goto out;
> +		}
> +
> +		/*
> +		 * The alternates file may already exist, e.g. when it has been
> +		 * seeded from a template directory. Read any preexisting
> +		 * entries so that we don't end up writing duplicates.
> +		 */
> +		orig = fopen(path.buf, "r");
> +		if (orig) {
> +			while (strbuf_getline(&line, orig) != EOF) {
> +				strset_add(&seen, line.buf);
> +				fprintf(alternates, "%s\n", line.buf);
> +			}
> +
> +			if (ferror(orig)) {
> +				ret = error_errno(_("unable to read alternates file"));
> +				fclose(orig);
> +				goto out;
> +			}

Shouldn't this be checked inside the for loop with every `fprintf` call?

> +
> +			fclose(orig);
> +		} else if (errno != ENOENT) {
> +			ret = error_errno(_("unable to read alternates file"));
> +			goto out;
> +		}
> +
> +		for (size_t i = 0; i < opts->alternates->nr; i++) {
> +			const char *alternate = opts->alternates->v[i];
> +			if (!strset_add(&seen, alternate))
> +				continue;
> +			fprintf(alternates, "%s\n", alternate);
> +		}
> +
> +		if (ferror(alternates)) {
> +			ret = error_errno(_("unable to write alternates file"));
> +			goto out;
> +		}
> +

same here.

> +		if (commit_lock_file(&alternates_lock)) {
> +			ret = error_errno(_("unable to commit alternates file"));
> +			goto out;
> +		}
> +	}
> +
> +	/* Reprepare the object database to activate alternates. */
> +	odb_reprepare(source->odb);
> +
> +	ret = 0;
> +
> +out:
> +	rollback_lock_file(&alternates_lock);
> +	strbuf_release(&line);
>  	strbuf_release(&path);
> -	return 0;
> +	strset_clear(&seen);
> +	return ret;
>  }
>
>  static void odb_source_files_prepare(struct odb_source *source,
> diff --git a/odb/source.h b/odb/source.h
> index ea8675247e..63f1c0c531 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -36,6 +36,15 @@ struct object_id;
>  struct odb_stream;
>  struct strvec;
>
> +struct odb_create_on_disk_options {
> +	/*
> +	 * Alternates that shall be written into the newly created object
> +	 * database. Whether or not this option can be handled is specific to
> +	 * the backend.
> +	 */

Would it make sense to formalize errors thrown by backends, so we know
when a backend specifically cannot handle alternates?

> +	const struct strvec *alternates;
> +};
> +

[snip]