From c96060b0cef79c9d76eb97965e700beb9651f35b Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 12 Oct 2022 12:52:35 +0000 Subject: bundle: properly clear all revision flags The verify_bundle() method checks two things for a bundle's prerequisites: 1. Are these objects in the object store? 2. Are these objects reachable from our references? In this second question, multiple uses of verify_bundle() in the same process can report an invalid bundle even though it is correct. The reason is due to not clearing all of the commit marks on the commits previously walked. The revision walk machinery was first introduced in-process by fb9a54150d3 (git-bundle: avoid fork() in verify_bundle(), 2007-02-22). This implementation used "-1" as the set of flags to clear. The next meaningful change came in 2b064697a5b (revision traversal: retire BOUNDARY_SHOW, 2007-03-05), which introduced the PREREQ_MARK flag instead of a flag normally controlled by the revision-walk machinery. In 86a0a408b90 (commit: factor out clear_commit_marks_for_object_array, 2011-10-01), the loop over the array of commits was replaced with a new clear_commit_marks_for_object_array(), but simultaneously the "-1" value was replaced with "ALL_REV_FLAGS", which stopped un-setting the PREREQ_MARK flag. This means that if multiple commits were marked by the PREREQ_MARK in a previous run of verify_bundle(), then this loop could terminate early due to 'i' going to zero: while (i && (commit = get_revision(&revs))) if (commit->object.flags & PREREQ_MARK) i--; The flag clearing work was changed again in 63647391e6c (bundle: avoid using the rev_info flag leak_pending, 2017-12-25), but that was only cosmetic and did not change the behavior. It may seem that it would be sufficient to add the PREREQ_MARK flag to the clear_commit_marks() call in its current location. However, we actually need to do it in the "cleanup:" step, since the first loop checking "Are these objects in the object store?" might add the PREREQ_MARK flag to some objects and then terminate without performing a walk due to one missing object. By clearing the flags in all cases, we avoid this issue when running verify_bundle() multiple times in the same process. Moving this loop to the cleanup step alone would cause a segfault when running 'git bundle verify' outside of a repository, but this is because of that error condition using "goto cleanup" when returning is perfectly safe. Nothing has been initialized at that point, so we can return immediately without causing any leaks. This behavior is verified carefully by a test that will be added soon when Git learns to download bundle lists in a 'git clone --bundle-uri' command. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- bundle.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) (limited to 'bundle.c') diff --git a/bundle.c b/bundle.c index 0208e6d90d..c277f3b936 100644 --- a/bundle.c +++ b/bundle.c @@ -202,10 +202,8 @@ int verify_bundle(struct repository *r, int i, ret = 0, req_nr; const char *message = _("Repository lacks these prerequisite commits:"); - if (!r || !r->objects || !r->objects->odb) { - ret = error(_("need a repository to verify a bundle")); - goto cleanup; - } + if (!r || !r->objects || !r->objects->odb) + return error(_("need a repository to verify a bundle")); repo_init_revisions(r, &revs, NULL); for (i = 0; i < p->nr; i++) { @@ -250,15 +248,6 @@ int verify_bundle(struct repository *r, error("%s %s", oid_to_hex(oid), name); } - /* 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); - } - if (verbose) { struct string_list *r; @@ -287,6 +276,14 @@ int verify_bundle(struct repository *r, list_objects_filter_spec(&header->filter)); } 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; } -- cgit v1.3 From 89bd7fedf947484da08e2722d663fdac23a431be Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 12 Oct 2022 12:52:37 +0000 Subject: bundle: add flags to verify_bundle() The verify_bundle() method has a 'verbose' option, but we will want to extend this method to have more granular control over its output. First, replace this 'verbose' option with a new 'flags' option with a single possible value: VERIFY_BUNDLE_VERBOSE. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/bundle.c | 5 +++-- bundle-uri.c | 7 ++++++- bundle.c | 9 +++++---- bundle.h | 14 ++++++++++++-- transport.c | 2 +- 5 files changed, 27 insertions(+), 10 deletions(-) (limited to 'bundle.c') diff --git a/builtin/bundle.c b/builtin/bundle.c index 2adad545a2..7d983a238f 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -119,7 +119,8 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) { goto cleanup; } close(bundle_fd); - if (verify_bundle(the_repository, &header, !quiet)) { + if (verify_bundle(the_repository, &header, + quiet ? 0 : VERIFY_BUNDLE_VERBOSE)) { ret = 1; goto cleanup; } @@ -185,7 +186,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) strvec_pushl(&extra_index_pack_args, "-v", "--progress-title", _("Unbundling objects"), NULL); ret = !!unbundle(the_repository, &header, bundle_fd, - &extra_index_pack_args) || + &extra_index_pack_args, 0) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); cleanup: diff --git a/bundle-uri.c b/bundle-uri.c index 70bfd2defe..d9060be707 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -303,7 +303,12 @@ static int unbundle_from_file(struct repository *r, const char *file) if ((bundle_fd = read_bundle_header(file, &header)) < 0) return 1; - if ((result = unbundle(r, &header, bundle_fd, NULL))) + /* + * Skip the reachability walk here, since we will be adding + * a reachable ref pointing to the new tips, which will reach + * the prerequisite commits. + */ + if ((result = unbundle(r, &header, bundle_fd, NULL, 0))) return 1; /* diff --git a/bundle.c b/bundle.c index c277f3b936..1f6a7f782e 100644 --- a/bundle.c +++ b/bundle.c @@ -189,7 +189,7 @@ static int list_refs(struct string_list *r, int argc, const char **argv) int verify_bundle(struct repository *r, struct bundle_header *header, - int verbose) + enum verify_bundle_flags flags) { /* * Do fast check, then if any prereqs are missing then go line by line @@ -248,7 +248,7 @@ int verify_bundle(struct repository *r, error("%s %s", oid_to_hex(oid), name); } - if (verbose) { + if (flags & VERIFY_BUNDLE_VERBOSE) { struct string_list *r; r = &header->references; @@ -617,7 +617,8 @@ err: } int unbundle(struct repository *r, struct bundle_header *header, - int bundle_fd, struct strvec *extra_index_pack_args) + int bundle_fd, struct strvec *extra_index_pack_args, + enum verify_bundle_flags flags) { struct child_process ip = CHILD_PROCESS_INIT; strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL); @@ -631,7 +632,7 @@ int unbundle(struct repository *r, struct bundle_header *header, strvec_clear(extra_index_pack_args); } - if (verify_bundle(r, header, 0)) + if (verify_bundle(r, header, flags)) return -1; ip.in = bundle_fd; ip.no_stdout = 1; diff --git a/bundle.h b/bundle.h index 0c052f5496..6652e81998 100644 --- a/bundle.h +++ b/bundle.h @@ -29,7 +29,13 @@ int read_bundle_header_fd(int fd, struct bundle_header *header, int create_bundle(struct repository *r, const char *path, int argc, const char **argv, struct strvec *pack_options, int version); -int verify_bundle(struct repository *r, struct bundle_header *header, int verbose); + +enum verify_bundle_flags { + VERIFY_BUNDLE_VERBOSE = (1 << 0), +}; + +int verify_bundle(struct repository *r, struct bundle_header *header, + enum verify_bundle_flags flags); /** * Unbundle after reading the header with read_bundle_header(). @@ -40,9 +46,13 @@ int verify_bundle(struct repository *r, struct bundle_header *header, int verbos * Provide "extra_index_pack_args" to pass any extra arguments * (e.g. "-v" for verbose/progress), NULL otherwise. The provided * "extra_index_pack_args" (if any) will be strvec_clear()'d for you. + * + * Before unbundling, this method will call verify_bundle() with the + * given 'flags'. */ int unbundle(struct repository *r, struct bundle_header *header, - int bundle_fd, struct strvec *extra_index_pack_args); + int bundle_fd, struct strvec *extra_index_pack_args, + enum verify_bundle_flags flags); int list_bundle_refs(struct bundle_header *header, int argc, const char **argv); diff --git a/transport.c b/transport.c index b51e991e44..de4d88687c 100644 --- a/transport.c +++ b/transport.c @@ -178,7 +178,7 @@ static int fetch_refs_from_bundle(struct transport *transport, if (!data->get_refs_from_bundle_called) get_refs_from_bundle_inner(transport); ret = unbundle(the_repository, &data->header, data->fd, - &extra_index_pack_args); + &extra_index_pack_args, 0); transport->hash_algo = data->header.hash_algo; return ret; } -- cgit v1.3 From 70334fc3ebf1c6199014d82bbbf0595b64a8fa90 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 12 Oct 2022 12:52:38 +0000 Subject: bundle-uri: quiet failed unbundlings When downloading a list of bundles in "all" mode, Git has no understanding of the dependencies between the bundles. Git attempts to unbundle the bundles in some order, but some may not pass the verify_bundle() step because of missing prerequisites. This is passed as error messages to the user, even when they eventually succeed in later attempts after their dependent bundles are unbundled. Add a new VERIFY_BUNDLE_QUIET flag to verify_bundle() that avoids the error messages from the missing prerequisite commits. The method still returns the number of missing prerequisit commits, allowing callers to unbundle() to notice that the bundle failed to apply. Use this flag in bundle-uri.c and test that the messages go away for 'git clone --bundle-uri' commands. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/bundle.c | 2 +- bundle-uri.c | 3 ++- bundle.c | 10 ++++++++-- bundle.h | 1 + t/t5558-clone-bundle-uri.sh | 25 ++++++++++++++++++++----- 5 files changed, 32 insertions(+), 9 deletions(-) (limited to 'bundle.c') diff --git a/builtin/bundle.c b/builtin/bundle.c index 7d983a238f..fd4586b09e 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -120,7 +120,7 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) { } close(bundle_fd); if (verify_bundle(the_repository, &header, - quiet ? 0 : VERIFY_BUNDLE_VERBOSE)) { + quiet ? VERIFY_BUNDLE_QUIET : VERIFY_BUNDLE_VERBOSE)) { ret = 1; goto cleanup; } diff --git a/bundle-uri.c b/bundle-uri.c index d9060be707..d872acf5ab 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -308,7 +308,8 @@ static int unbundle_from_file(struct repository *r, const char *file) * a reachable ref pointing to the new tips, which will reach * the prerequisite commits. */ - if ((result = unbundle(r, &header, bundle_fd, NULL, 0))) + if ((result = unbundle(r, &header, bundle_fd, NULL, + VERIFY_BUNDLE_QUIET))) return 1; /* diff --git a/bundle.c b/bundle.c index 1f6a7f782e..4ef7256aa1 100644 --- a/bundle.c +++ b/bundle.c @@ -216,7 +216,10 @@ int verify_bundle(struct repository *r, add_pending_object(&revs, o, name); continue; } - if (++ret == 1) + ret++; + if (flags & VERIFY_BUNDLE_QUIET) + continue; + if (ret == 1) error("%s", message); error("%s %s", oid_to_hex(oid), name); } @@ -243,7 +246,10 @@ int verify_bundle(struct repository *r, assert(o); /* otherwise we'd have returned early */ if (o->flags & SHOWN) continue; - if (++ret == 1) + ret++; + if (flags & VERIFY_BUNDLE_QUIET) + continue; + if (ret == 1) error("%s", message); error("%s %s", oid_to_hex(oid), name); } diff --git a/bundle.h b/bundle.h index 6652e81998..575c34245d 100644 --- a/bundle.h +++ b/bundle.h @@ -32,6 +32,7 @@ int create_bundle(struct repository *r, const char *path, enum verify_bundle_flags { VERIFY_BUNDLE_VERBOSE = (1 << 0), + VERIFY_BUNDLE_QUIET = (1 << 1), }; int verify_bundle(struct repository *r, struct bundle_header *header, diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh index a86dc04f52..9b15907838 100755 --- a/t/t5558-clone-bundle-uri.sh +++ b/t/t5558-clone-bundle-uri.sh @@ -99,7 +99,10 @@ test_expect_success 'clone bundle list (file, no heuristic)' ' uri = file://$(pwd)/clone-from/bundle-4.bundle EOF - git clone --bundle-uri="file://$(pwd)/bundle-list" clone-from clone-list-file && + git clone --bundle-uri="file://$(pwd)/bundle-list" \ + clone-from clone-list-file 2>err && + ! grep "Repository lacks these prerequisite commits" err && + git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-list-file cat-file --batch-check err && + ! grep "Repository lacks these prerequisite commits" err && + git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-all-some cat-file --batch-check err && + ! grep "Repository lacks these prerequisite commits" err && + git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-all-fail cat-file --batch-check err && + ! grep "Repository lacks these prerequisite commits" err && + git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-any-file cat-file --batch-check err && + ! grep "Repository lacks these prerequisite commits" err && + git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-list-http cat-file --batch-check