Re: [PATCH v6 3/3] unbundle: support object verification for fetches
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 11, 2024, 20:05 UTC
- Message-ID
- <xmqqsexjtfcg.fsf@gitster.g>
- In-Reply-To
- <53395e8c08a8487f3e53dca15766307854a24b3b.1718109943.git.gitgitgadget@gmail.com>
"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> From: Xing Xin <xingxin.xx@bytedance.com> > > This commit extends object verification support for fetches in > `bundle.c:unbundle` by adding the `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH` > option to `verify_bundle_flags`. When this option is enabled, > `bundle.c:unbundle` invokes `fetch-pack.c:fetch_pack_fsck_objects` to > determine whether to append the "--fsck-objects" flag to > "git-index-pack".
Please start your proposed log message by stating what the perceived problem without this patch in the current world is. Without it, you cannot easily answer the most basic question: "why are we doing this?"
The name VERIFY_BUNDLE_FSCK_FOLLOW_FETCH does not read very well. VERIFY_BUNDLE part is common across various flags, so what is specific to the feature is "FSCK_FOLLOW_FETCH", and it is good that we convey the fact that we do a bit more than the normal VERIFY_BUNDLE (which is, to read the prerequisite headers and make sure we have them in the sense that they are reachable from our refs) with the word FSCK.
But is it necessary (or even a good idea) to limit its usability with "FOLLOW_FETCH" (which does not look even grammatical)? Aren't we closing the door to other folks who may want to do a more thorough fsck-like checks in other codepaths by saying "if you are not doing this immediately after you fetch, you are unwelcome to use this flag"?
Show 32 quoted lines
> `VERIFY_BUNDLE_FSCK_FOLLOW_FETCH` is now passed to `unbundle` in the > fetching process, including: > > - `transport.c:fetch_refs_from_bundle` for direct bundle fetches. > - `bundle-uri.c:unbundle_from_file` for bundle-uri enabled fetches. > > This addition ensures a consistent logic for object verification during > fetch operations. Tests have been added to confirm functionality in the > scenarios mentioned above. > > Reviewed-by: Patrick Steinhardt <ps@pks.im> > Signed-off-by: Xing Xin <xingxin.xx@bytedance.com> > --- > bundle-uri.c | 2 +- > bundle.c | 5 +++++ > bundle.h | 1 + > t/t5558-clone-bundle-uri.sh | 35 ++++++++++++++++++++++++++++++++++- > t/t5607-clone-bundle.sh | 33 +++++++++++++++++++++++++++++++++ > transport.c | 2 +- > 6 files changed, 75 insertions(+), 3 deletions(-) > > diff --git a/bundle-uri.c b/bundle-uri.c > index 65666a11d9c..e7ebac6ce57 100644 > --- a/bundle-uri.c > +++ b/bundle-uri.c > @@ -373,7 +373,7 @@ static int unbundle_from_file(struct repository *r, const char *file) > * the prerequisite commits. > */ > if ((result = unbundle(r, &header, bundle_fd, NULL, > - VERIFY_BUNDLE_QUIET))) > + VERIFY_BUNDLE_QUIET | VERIFY_BUNDLE_FSCK_FOLLOW_FETCH))) > return 1;
OK (modulo the flag name).
> + if (flags & VERIFY_BUNDLE_FSCK_FOLLOW_FETCH) > + if (fetch_pack_fsck_objects()) > + strvec_push(&ip.args, "--fsck-objects"); > +
OK, that's quite straight-forward. We are running "index-pack --fix-thin --stdin" and feeding the bundle data to it. We just tell it to also work in the "--fsck-objects" mode.
Show 10 quoted lines
> diff --git a/transport.c b/transport.c > index 0ad04b77fd2..6cd5683bb45 100644 > --- a/transport.c > +++ b/transport.c > @@ -184,7 +184,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, 0); > + &extra_index_pack_args, VERIFY_BUNDLE_FSCK_FOLLOW_FETCH);
OK.
I wonder if something like this is a potential follow-up topic somebody may be interested in after the dust settles. That is exactly why the name of this bit matters.
builtin/bundle.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git c/builtin/bundle.c w/builtin/bundle.c index d5d41a8f67..eeb5963dcb 100644 --- c/builtin/bundle.c +++ w/builtin/bundle.c @@ -194,10 +194,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) int bundle_fd = -1; int ret; int progress = isatty(2); + int fsck_objects = 0; struct option options[] = { OPT_BOOL(0, "progress", &progress, N_("show progress meter")), + OPT_BOOL(0, "fsck-objects", &fsck_objects, + N_("check the objects in the bundle")), OPT_END() }; char *bundle_file; @@ -217,7 +220,8 @@ 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, 0) || + &extra_index_pack_args, + fsck_objects ? VERIFY_BUNDLE_FSCK_FOLLOW_FETCH : 0) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); cleanup: