undef/git-commit-vandalism
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

bundle: verify using check_connected()

Browse Source 
When Git verifies a bundle to see if it is safe for unbundling, it first
looks to see if the prerequisite commits are in the object store. This
is an easy way to "fail fast" but it is not a sufficient check for
updating refs that guarantee closure under reachability. There could
still be issues if those commits are not reachable from the repository's
references. The repository only has guarantees that its object store is
closed under reachability for the objects that are reachable from
references.

Thus, the code in verify_bundle() has previously had the additional
check that all prerequisite commits are reachable from repository
references. This is done via a revision walk from all references,
stopping only if all prerequisite commits are discovered or all commits
are walked. This uses a custom walk to verify_bundle().

This check is more strict than what Git applies to fetched pack-files.
In the fetch case, Git guarantees that the new references are closed
under reachability by walking from the new references until walking
commits that are reachable from repository refs. This is done through
the well-used check_connected() method.

To better align with the restrictions required by 'git fetch',
reimplement this check in verify_bundle() to use check_connected(). This
also simplifies the code significantly.

The previous change added a test that verified the behavior of 'git
bundle verify' and 'git bundle unbundle' in this case, and the error
messages looked like this:

  error: Could not read <missing-commit>
  fatal: Failed to traverse parents of commit <extant-commit>

However, by changing the revision walk slightly within check_connected()
and using its quiet mode, we can omit those messages. Instead, we get
only this message, tailored to describing the current state of the
repository:

  error: some prerequisite commits exist in the object store,
         but are not connected to the repository's history

(Line break added here for the commit message formatting, only.)

While this message does not include any object IDs, there is no
guarantee that those object IDs would help the user diagnose what is
going on, as they could be separated from the prerequisite commits by
some distance. At minimum, this situation describes the situation in a
more informative way than the previous error messages.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Derrick Stolee 2023-01-31 13:29:10 +00:00 committed by Junio C Hamano
parent e72171f085
commit d9fd674c8b
2 changed files with 33 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

75
bundle.c
View File

@ -12,6 +12,7 @@
#include "refs.h" #include "refs.h"
#include "strvec.h" #include "strvec.h"
#include "list-objects-filter-options.h" #include "list-objects-filter-options.h"
#include "connected.h"
static const char v2_bundle_signature[] = "# v2 git bundle\n"; static const char v2_bundle_signature[] = "# v2 git bundle\n";
static const char v3_bundle_signature[] = "# v3 git bundle\n"; static const char v3_bundle_signature[] = "# v3 git bundle\n";
@ -187,6 +188,21 @@ static int list_refs(struct string_list *r, int argc, const char **argv)
/* Remember to update object flag allocation in object.h */ /* Remember to update object flag allocation in object.h */
#define PREREQ_MARK (1u<<16) #define PREREQ_MARK (1u<<16)
struct string_list_iterator {
struct string_list *list;
size_t cur;
};
static const struct object_id *iterate_ref_map(void *cb_data)
{
struct string_list_iterator *iter = cb_data;
if (iter->cur >= iter->list->nr)
return NULL;
return iter->list->items[iter->cur++].util;
}
int verify_bundle(struct repository *r, int verify_bundle(struct repository *r,
struct bundle_header *header, struct bundle_header *header,
enum verify_bundle_flags flags) enum verify_bundle_flags flags)
@ -196,26 +212,25 @@ int verify_bundle(struct repository *r,
* to be verbose about the errors * to be verbose about the errors
*/ */
struct string_list *p = &header->prerequisites; struct string_list *p = &header->prerequisites;
struct rev_info revs = REV_INFO_INIT; int i, ret = 0;
const char *argv[] = {NULL, "--all", NULL};
struct commit *commit;
int i, ret = 0, req_nr;
const char *message = _("Repository lacks these prerequisite commits:"); const char *message = _("Repository lacks these prerequisite commits:");
struct string_list_iterator iter = {
.list = p,
};
struct check_connected_options opts = {
.quiet = 1,
};
if (!r || !r->objects || !r->objects->odb) if (!r || !r->objects || !r->objects->odb)
return error(_("need a repository to verify a bundle")); return error(_("need a repository to verify a bundle"));
repo_init_revisions(r, &revs, NULL);
for (i = 0; i < p->nr; i++) { for (i = 0; i < p->nr; i++) {
struct string_list_item *e = p->items + i; struct string_list_item *e = p->items + i;
const char *name = e->string; const char *name = e->string;
struct object_id *oid = e->util; struct object_id *oid = e->util;
struct object *o = parse_object(r, oid); struct object *o = parse_object(r, oid);
if (o) { if (o)
o->flags |= PREREQ_MARK;
add_pending_object(&revs, o, name);
continue; continue;
}
ret++; ret++;
if (flags & VERIFY_BUNDLE_QUIET) if (flags & VERIFY_BUNDLE_QUIET)
continue; continue;
@ -223,37 +238,14 @@ int verify_bundle(struct repository *r,
error("%s", message); error("%s", message);
error("%s %s", oid_to_hex(oid), name); error("%s %s", oid_to_hex(oid), name);
} }
if (revs.pending.nr != p->nr) if (ret)
goto cleanup; goto cleanup;
req_nr = revs.pending.nr;
setup_revisions(2, argv, &revs, NULL);
list_objects_filter_copy(&revs.filter, &header->filter); if ((ret = check_connected(iterate_ref_map, &iter, &opts)))
error(_("some prerequisite commits exist in the object store, "
if (prepare_revision_walk(&revs)) "but are not connected to the repository's history"));
die(_("revision walk setup failed"));
i = req_nr;
while (i && (commit = get_revision(&revs)))
if (commit->object.flags & PREREQ_MARK)
i--;
for (i = 0; i < p->nr; i++) {
struct string_list_item *e = p->items + i;
const char *name = e->string;
const struct object_id *oid = e->util;
struct object *o = parse_object(r, oid);
assert(o); /* otherwise we'd have returned early */
if (o->flags & SHOWN)
continue;
ret++;
if (flags & VERIFY_BUNDLE_QUIET)
continue;
if (ret == 1)
error("%s", message);
error("%s %s", oid_to_hex(oid), name);
}
/* TODO: preserve this verbose language. */
if (flags & VERIFY_BUNDLE_VERBOSE) { if (flags & VERIFY_BUNDLE_VERBOSE) {
struct string_list *r; struct string_list *r;
@ -282,15 +274,6 @@ int verify_bundle(struct repository *r,
list_objects_filter_spec(&header->filter)); list_objects_filter_spec(&header->filter));
} }
cleanup: cleanup:
/* Clean up objects used, as they will be reused. */
for (i = 0; i < p->nr; i++) {
struct string_list_item *e = p->items + i;
struct object_id *oid = e->util;
commit = lookup_commit_reference_gently(r, oid, 1);
if (commit)
clear_commit_marks(commit, ALL_REV_FLAGS | PREREQ_MARK);
}
release_revisions(&revs);
return ret; return ret;
} }

8
t/t6020-bundle-misc.sh
View File

@ -588,14 +588,14 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
# Verify should fail # Verify should fail
test_must_fail git bundle verify \ test_must_fail git bundle verify \
../clone-from/tip.bundle 2>err && ../clone-from/tip.bundle 2>err &&
grep "Could not read $BAD_OID" err && grep "some prerequisite commits .* are not connected" err &&
grep "Failed to traverse parents of commit $TIP_OID" err && test_line_count = 1 err &&
# Unbundling should fail # Unbundling should fail
test_must_fail git bundle unbundle \ test_must_fail git bundle unbundle \
../clone-from/tip.bundle 2>err && ../clone-from/tip.bundle 2>err &&
grep "Could not read $BAD_OID" err && grep "some prerequisite commits .* are not connected" err &&
grep "Failed to traverse parents of commit $TIP_OID" err test_line_count = 1 err
) )
' '