index-pack: teach --promisor to forbid pack name

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] and [2], teaching --promisor to 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.

Currently, Git uses "index-pack --promisor" only when fetching into
a repo, so it could be argued that we should teach "index-pack" a
new argument (say, "--fetching-mode") instead of tying --promisor to
a generic argument like the packfile name. However, this --promisor
feature could conceivably be used whenever we have a packfile that is
known to come from the promisor remote (whether obtained through Git's
fetch protocol or through other means) so not using a new argument seems
reasonable - one could envision a user-made script obtaining a packfile
and then running "index-pack --promisor --stdin", for example. In fact,
it might be possible to relax the restriction further (say, by also
allowing --promisor when indexing a packfile that is in the object DB),
but relaxing the restriction is backwards-compatible so we can revisit
that later.

One thing to watch out for is the possibility of a future Git feature
that indexes a pack in the context of a repo, but does not necessarily
write the resulting pack to it (and does not necessarily desire to
make any changes to the object DB). One such feature would be fetch
quarantine, which might need the repo context in order to detect
hash collisions, but would also need to ensure that the object DB
is undisturbed in case the fetch fails for whatever reason, even if
the reason occurs only after the indexing is complete. It may not be
obvious to the implementer of such a feature that "index-pack" could
sometimes write packs other than the indexed pack to the object DB,
but there are already other ways that "fetch" could write to the object
DB (in particular, packfile URIs and bundle URIs), so hopefully the
implementation of this future feature would already include a test that
the object DB be undisturbed.

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.)

[1] https://lore.kernel.org/git/20241114005652.GC1140565@coredump.intra.peff.net/
[2] https://lore.kernel.org/git/20241119185345.GB15723@coredump.intra.peff.net/

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 4be09e58e721453a2250a361ee1fa7bc3c08c7e8..58dd5b5f0e882f70c5d0ef4cff512d21745a9698 100644 (file)
--- 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 <pack-file> to not be specified.
 
 NOTES
 -----

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 08b340552f3e92b5c56de24a37fea7413dd2306d..05758a2f3e61efcfbf44d111f391344aa23b840b 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1970,6 +1970,8 @@ 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 && pack_name)
+               die(_("--promisor cannot be used with a pack name"));
        if (from_stdin && !startup_info->have_repository)
                die(_("--stdin requires a git repository"));
        if (from_stdin && hash_algo)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index aff164ddf8ec79c23d0b230daa38e3892b02a233..c53f355e48ced4f9e41307baf1a45bf5a9261f64 100755 (executable)
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -332,10 +332,8 @@ test_expect_success 'build pack index for an existing pack' '
        git index-pack -o tmp.idx test-3.pack &&
        cmp tmp.idx test-1-${packname_1}.idx &&
 
-       git index-pack --promisor=message test-3.pack &&
+       git index-pack test-3.pack &&
        cmp test-3.idx test-1-${packname_1}.idx &&
-       echo message >expect &&
-       test_cmp expect test-3.promisor &&
 
        cat test-2-${packname_2}.pack >test-3.pack &&
        git index-pack -o tmp.idx test-2-${packname_2}.pack &&