Re: [PATCH v3 02/11] bundle: verify using check_connected()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 31, 2023, 17:35 UTC
- Message-ID
- <xmqqr0vay5cz.fsf@gitster.g>
- In-Reply-To
- <20c29d37f9c1ba1367145331d25dd27f966312cd.1675171759.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> 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.
As I often say, breaking repository faster is not the kind of performance gain we want to have in Git, and I am in favor of this iteration compared to the earlier version that mostly punted on ensuring the correctness (rather, it relied on assumption that "most of the time this should be OK").
> bundle.c | 75 ++++++++++++++++-------------------------- > t/t6020-bundle-misc.sh | 8 ++--- > 2 files changed, 33 insertions(+), 50 deletions(-)
The diffstat is very pleasing to see.
Let me read the postimage along aloud (preimage omitted).
Show 22 quoted lines
> diff --git a/bundle.c b/bundle.c
> index 4ef7256aa11..76c3a904898 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -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 */
> #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;
> +}This is to let check_connected() collect all the prerequisite object names. OK.
Show 37 quoted lines
> int verify_bundle(struct repository *r,
> struct bundle_header *header,
> enum verify_bundle_flags flags)
> {
> /*
> * Do fast check, then if any prereqs are missing then go line by line
> * to be verbose about the errors
> */
> struct string_list *p = &header->prerequisites;
> + int i, ret = 0;
> 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)
> return error(_("need a repository to verify a bundle"));
>
> for (i = 0; i < p->nr; i++) {
> struct string_list_item *e = p->items + i;
> const char *name = e->string;
> struct object_id *oid = e->util;
> struct object *o = parse_object(r, oid);
> + if (o)
> continue;
> ret++;
> if (flags & VERIFY_BUNDLE_QUIET)
> continue;
> if (ret == 1)
> error("%s", message);
> error("%s %s", oid_to_hex(oid), name);
> }
> + if (ret)
> goto cleanup;The "quick fail" logic as before. Looking sensible.
>
> + if ((ret = check_connected(iterate_ref_map, &iter, &opts)))
> + error(_("some prerequisite commits exist in the object store, "
> + "but are not connected to the repository's history"));And then we let check_connected() to ensure that traversing from these prerequisite objects down to the DAG formed by existing refs will not die from missing objects. Makes sense.
> + /* TODO: preserve this verbose language. */
I am lost -- aren't we preserving the BUNDLE_VERBOSE code below?
> if (flags & VERIFY_BUNDLE_VERBOSE) {Show 22 quoted lines
> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh > index 38dbbf89155..7d40994991e 100755 > --- a/t/t6020-bundle-misc.sh > +++ b/t/t6020-bundle-misc.sh > @@ -595,14 +595,14 @@ test_expect_success 'verify catches unreachable, broken prerequisites' ' > # Verify should fail > test_must_fail git bundle verify \ > ../clone-from/tip.bundle 2>err && > - grep "Could not read $BAD_OID" err && > - grep "Failed to traverse parents of commit $TIP_OID" err && > + grep "some prerequisite commits .* are not connected" err && > + test_line_count = 1 err && > > # Unbundling should fail > test_must_fail git bundle unbundle \ > ../clone-from/tip.bundle 2>err && > - grep "Could not read $BAD_OID" err && > - grep "Failed to traverse parents of commit $TIP_OID" err > + grep "some prerequisite commits .* are not connected" err && > + test_line_count = 1 err > ) > '
Especially with the new test added in the previous step, we know we are not trading correctness off. Excellent.
I wonder how much the performance hit does this version incur over the "not safe at all" version and over the "use custom and stricter-than-needed" version, by the way?
Thanks.