builtin/index-pack: fix deferred fsck outside repos

When asked to perform object consistency checks via the `--fsck-objects`
flag we verify that each object part of the pack is valid. In general,
this check can even be performed outside of a Git repository: we don't
need an initialized object database as we simply read the object from
the packfile directly.

But there's one exception: a subset of the object checks may be deferred
to a later point in time. For now, this only concerns ".gitmodules" and
".gitattributes" files: whenever we see a tree referencing these files
we queue them for a deferred check. This is done because we need to do
some extra checks for those files to ensure that they are well-formed,
and these checks need to be done regardless of whether the corresponding
blobs are part of the packfile or not.

This works inside a repository, but unfortunately the logic leads to a
segfault when running outside of one. This is because we eventually call
`odb_read_object()`, which will crash because the object database has
not been initialized.

There's multiple options here:

  - We could in theory create a purely in-memory database with only a
    packfile store that contains the single packfile. We don't really
    have the infrastructure for this yet though, and it would end up
    being quite hacky.

  - We could refuse to perform consistency checks outside of a
    repository. But most of the checks work alright, so this would be a
    regression.

  - We can skip the finalizing consistency checks when running outside
    of a repository. This is not as invasive as skipping all checks,
    but it's not great to randomly skip a subset of tests, either.

None of these options really feel perfect. The first one would be the
obvious choice if easily possible.

There's another option though: instead of skipping the final object
checks, we can die if there are any queued object checks. With this
change we now die exactly if and only if we would have previously
segfaulted. Like this we ensure that objects that _may_ fail the
consistency checks won't be silently skipped, and at the same time we
give users a much better error message.

Refactor the code accordingly and add a test that would have triggered
the segfault. Note that we also move down the logic to add the packfile
to the store. There is no point doing this any earlier than right before
we execute `fsck_finish()`, and it ensures that the logic to set up and
perform the consistency check is self-contained.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2b78ba7fe4d14a8bcb12165395ff02deeb8c047f..699fe678cd60b0af8b7edf77f2204a064140d94a 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1640,7 +1640,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
        rename_tmp_packfile(&final_index_name, curr_index_name, &index_name,
                            hash, "idx", 1);
 
-       if (do_fsck_object)
+       if (do_fsck_object && startup_info->have_repository)
                packfile_store_load_pack(the_repository->objects->packfiles,
                                         final_index_name, 0);
 
@@ -2110,8 +2110,23 @@ int cmd_index_pack(int argc,
        else
                close(input_fd);
 
-       if (do_fsck_object && fsck_finish(&fsck_options))
-               die(_("fsck error in pack objects"));
+       if (do_fsck_object) {
+               /*
+                * We cannot perform queued consistency checks when running
+                * outside of a repository because those require us to read
+                * from the object database, which is uninitialized.
+                *
+                * TODO: we may eventually set up an in-memory object database,
+                * which would allow us to perform these queued checks.
+                */
+               if (!startup_info->have_repository &&
+                   fsck_has_queued_checks(&fsck_options))
+                       die(_("cannot perform queued object checks outside "
+                             "of a repository"));
+
+               if (fsck_finish(&fsck_options))
+                       die(_("fsck error in pack objects"));
+       }
 
        free(opts.anomaly);
        free(objects);

diff --git a/fsck.c b/fsck.c
index 341e100d24ece03992d9b10d6c91c5bea089b752..8e1565fe6d013340307bd888b1b203ae37efe88e 100644 (file)
--- a/fsck.c
+++ b/fsck.c
@@ -1350,6 +1350,12 @@ int fsck_finish(struct fsck_options *options)
        return ret;
 }
 
+bool fsck_has_queued_checks(struct fsck_options *options)
+{
+       return !oidset_equal(&options->gitmodules_found, &options->gitmodules_done) ||
+              !oidset_equal(&options->gitattributes_found, &options->gitattributes_done);
+}
+
 void fsck_options_clear(struct fsck_options *options)
 {
        free(options->msg_type);

diff --git a/fsck.h b/fsck.h
index cb6ef32f4f3aaa1ac2ff8a97e38ab9b60b4fadf9..336917c0451aacc74db1534d21f55ded0baeb8be 100644 (file)
--- a/fsck.h
+++ b/fsck.h
@@ -248,6 +248,13 @@ int fsck_tag_standalone(const struct object_id *oid, const char *buffer,
  */
 int fsck_finish(struct fsck_options *options);
 
+/*
+ * Check whether there are any checks that have been queued up and that still
+ * need to be run. Returns `false` iff `fsck_finish()` wouldn't perform any
+ * actions, `true` otherwise.
+ */
+bool fsck_has_queued_checks(struct fsck_options *options);
+
 /*
  * Clear the fsck_options struct, freeing any allocated memory.
  */

diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 413c99274c8f3029cf259ca60c70c31348da12b8..9697448cb276341b11d3541334aae4b88c668997 100755 (executable)
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -293,4 +293,20 @@ test_expect_success 'too-large packs report the breach' '
        grep "maximum allowed size (20 bytes)" err
 '
 
+# git-index-pack(1) uses the default hash algorithm outside of the repository,
+# and it has no way to tell it otherwise. So we can only run this test with the
+# default hash algorithm, as it would otherwise fail to parse the tree.
+test_expect_success DEFAULT_HASH_ALGORITHM 'index-pack --fsck-objects outside of a repo' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               printf "100644 blob $(test_oid 001)\t.gitattributes\n" >tree &&
+               git mktree --missing <tree >tree-oid &&
+               git pack-objects <tree-oid pack &&
+               test_must_fail nongit git index-pack --fsck-objects "$(pwd)"/pack-*.pack 2>err &&
+               test_grep "cannot perform queued object checks outside of a repository" err
+       )
+'
+
 test_done