Re: [PATCH] index-pack: teach --promisor to require --stdin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2024, 03:29 UTC
Message-ID: <xmqq1pz7gaua.fsf@gitster.g>
In-Reply-To: <20241118190210.772105-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 12 quoted lines

> Currently,
>
>  - Running "index-pack --promisor" outside a repo segfaults.
>  - It may be confusing to a user that running "index-pack --promisor"
>    within a repo may make changes to the repo's object DB, especially
>    since the packs indexed by the index-pack invocation may not even be
>    related to the repo.
>
> As discussed in [1], teaching --promisor to require --stdin and forbid a
> packfile name solves both these problems. This combination of arguments
> requires a repo (since we are writing the resulting .pack and .idx to
> it) and it is clear that the files are related to the repo.

Makes sense.

> This change requires the change to t5300 by 1f52cdfacb (index-pack:
> document and test the --promisor option, 2022-03-09) to be undone.
> (--promisor is already tested indirectly, so we don't need the explicit
> test here any more.)

OK.

Show 28 quoted lines

> This is on jt/repack-local-promisor.
> diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
> index 4be09e58e7..ac96935d73 100644
> --- a/Documentation/git-index-pack.txt
> +++ b/Documentation/git-index-pack.txt
> @@ -144,6 +144,8 @@ Also, if there are objects in the given pack that references non-promisor
>  objects (in the repo), repacks those non-promisor objects into a promisor
>  pack. This avoids a situation in which a repo has non-promisor objects that are
>  accessible through promisor objects.
> ++
> +Requires --stdin, and requires <pack-file> to not be specified.
>  
>  NOTES
>  -----
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 08b340552f..c46b6e4061 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,
>  		usage(index_pack_usage);
>  	if (fix_thin_pack && !from_stdin)
>  		die(_("the option '%s' requires '%s'"), "--fix-thin", "--stdin");
> +	if (promisor_msg && !from_stdin)
> +		die(_("the option '%s' requires '%s'"), "--promisor", "--stdin");
> +	if (promisor_msg && pack_name)
> +		die(_("--promisor cannot be used with a pack name"));
>  	if (from_stdin && !startup_info->have_repository)
>  		die(_("--stdin requires a git repository"));

OK.  Thanks, will queue.