Re: [PATCH v2 10/13] builtin/multi-pack-index: refuse unknown sources with "--object-dir="

[Justin Tobler <jltobler@gmail.com>]

On 26/09/02 03:34PM, Patrick Steinhardt wrote:
> Users can tell git-multi-pack-index(1) to access multi-pack indices that
> are stored in a different object directory via the "--object-dir="
> option. This allows them to for example write or verify a multi-pack
> index other than the one located in the main object directory in case a
> repository has alternates with multiple multi-pack indices.
> 
> But while the documentation explicitly points out that the specified
> object directory must be an alternate of the current repository, we
> never verify that property. Instead, starting with 017db7bb14 (midx:
> load multi-pack indices via their source, 2025-08-11), we now construct
> an ad-hoc source and link it to the main object directory.
> 
> Besides contradicting the documentation, it's dubious that this really
> ought to work in the first place: creating a multi-pack index (and
> potentially a bitmap) for a completely foreign object directory is of
> questionable value, as bitmap commit selection operates on the invoking
> repository's refs. Furthermore, this is the only remaining caller
> outside of our test helpers that constructs an ad-hoc source and links
> it to the database, and we want to get rid of this mechanism as part of
> this series.

I was curious if there was any intentional reason that 017db7bb14
started added these as an alternate source. I assume though the reason
was just to address the tests when we started loading multi-pack indexes
via sources. So aligning with the prexisting documentation makes sense
to me.

> Stop constructing the ad-hoc source and instead refuse the operation.
> While this results in a change in behaviour, this restriction has been
> documented as such ever since f57a739691 (midx: avoid opening multiple
> MIDXs when writing, 2021-09-01).
> 
> Note that this change requires us to adapt one test chain in t5319, as
> it creates an object directory that is not connected to any repository
> and then uses it via "--object-dir=". The setup itself already documents
> this and does the necessary gymnastics to link the object directory to a
> temporary repository, but subsequent tests don't. Adapt those tests to
> retain and reuse the temporary repository.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/multi-pack-index.c  | 3 ++-
>  t/t5319-multi-pack-index.sh | 9 ++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 6e73c85cde..753bd53a70 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -90,7 +90,8 @@ static struct odb_source_files *handle_object_dir_option(struct repository *repo
>  {
>  	struct odb_source *source = odb_find_source(repo->objects, opts.object_dir);
>  	if (!source)
> -		source = odb_add_to_alternates_memory(repo->objects, opts.object_dir);
> +		die(_("object directory is not an alternate of the current repository: '%s'"),
> +		    opts.object_dir);

Now we no longer add these as in-memory alternates sources. Looks good.

>  	return odb_source_files_downcast(source);
>  }
>  
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 68143cb5b7..00e90f163f 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -698,10 +698,9 @@ test_expect_success 'force some 64-bit offsets with pack-objects' '
>  	corrupt_data $idx64 $(test_oid idxoff) "\02" &&
>  	# objects64 is not a real repository, but can serve as an alternate
>  	# anyway so we can write a MIDX into it
> -	git init repo &&
> -	test_when_finished "rm -fr repo" &&

Ok, now we just reuse the properly set up repo. Make sense.

> +	git init repo64 &&
>  	(
> -		cd repo &&
> +		cd repo64 &&
>  		( cd ../objects64 && pwd ) >.git/objects/info/alternates &&
>  		midx64=$(git multi-pack-index --object-dir=../objects64 write)
>  	) &&
> @@ -709,7 +708,7 @@ test_expect_success 'force some 64-bit offsets with pack-objects' '
>  '
>  
>  test_expect_success 'verify multi-pack-index with 64-bit offsets' '
> -	git multi-pack-index verify --object-dir=objects64
> +	git -C repo64 multi-pack-index verify --object-dir=../objects64
>  '
>  
>  NUM_OBJECTS=63
> @@ -721,7 +720,7 @@ MIDX_BYTE_LARGE_OFFSET=$(($MIDX_OFFSET_LARGE_OFFSETS + 3))
>  
>  test_expect_success 'verify incorrect 64-bit offset' '
>  	corrupt_midx_and_verify $MIDX_BYTE_LARGE_OFFSET "\07" objects64 \
> -		"incorrect object offset"
> +		"incorrect object offset" "git -C repo64 multi-pack-index verify --object-dir=../objects64"
>  '
>  
>  test_expect_success 'setup expire tests' '
> 
> -- 
> 2.55.0.979.g7e5102b832.dirty
> 
>