"Xing Xin via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Xing Xin <xingxin.xx@xxxxxxxxxxxxx>
>
> 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"?
> `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@xxxxxx>
> Signed-off-by: Xing Xin <xingxin.xx@xxxxxxxxxxxxx>
> ---
> 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.
> 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:
