From: Junio C Hamano <gitster@pobox.com>
To: "Xing Xin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Patrick Steinhardt <ps@pks.im>,
Karthik Nayak <karthik.188@gmail.com>,
blanet <bupt_xingxin@163.com>,
Xing Xin <xingxin.xx@bytedance.com>
Subject: Re: [PATCH v6 3/3] unbundle: support object verification for fetches
Date: Tue, 11 Jun 2024 13:05:35 -0700 [thread overview]
Message-ID: <xmqqsexjtfcg.fsf@gitster.g> (raw)
In-Reply-To: <53395e8c08a8487f3e53dca15766307854a24b3b.1718109943.git.gitgitgadget@gmail.com> (Xing Xin via GitGitGadget's message of "Tue, 11 Jun 2024 12:45:43 +0000")
"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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"?
> `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.
> 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:
next prev parent reply other threads:[~2024-06-11 20:05 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-15 3:01 [PATCH] bundle-uri: refresh packed_git if unbundle succeed blanet via GitGitGadget
2024-05-17 5:00 ` Patrick Steinhardt
2024-05-17 16:14 ` Junio C Hamano
2024-05-20 11:48 ` Xing Xin
2024-05-20 17:19 ` Junio C Hamano
2024-05-27 16:04 ` Xing Xin
2024-05-20 9:41 ` Xing Xin
2024-05-17 7:36 ` Karthik Nayak
2024-05-20 10:19 ` Xing Xin
2024-05-20 12:36 ` [PATCH v2] bundle-uri: verify oid before writing refs blanet via GitGitGadget
2024-05-21 15:41 ` Karthik Nayak
2024-05-27 15:41 ` [PATCH v3 0/4] object checking related additions and fixes for bundles in fetches blanet via GitGitGadget
2024-05-27 15:41 ` [PATCH v3 1/4] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-05-28 11:55 ` Patrick Steinhardt
2024-05-30 8:32 ` Xing Xin
2024-05-27 15:41 ` [PATCH v3 2/4] unbundle: introduce unbundle_fsck_flags for fsckobjects handling Xing Xin via GitGitGadget
2024-05-28 12:03 ` Patrick Steinhardt
2024-05-29 18:12 ` Xing Xin
2024-05-30 4:38 ` Patrick Steinhardt
2024-05-30 8:46 ` Xing Xin
2024-05-27 15:41 ` [PATCH v3 3/4] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-05-28 12:03 ` Patrick Steinhardt
2024-05-28 17:10 ` Junio C Hamano
2024-05-28 17:24 ` Junio C Hamano
2024-05-29 5:52 ` Patrick Steinhardt
2024-05-30 8:48 ` Xing Xin
2024-05-29 5:52 ` Patrick Steinhardt
2024-05-27 15:41 ` [PATCH v3 4/4] unbundle: introduce new option UNBUNDLE_FSCK_FOLLOW_FETCH Xing Xin via GitGitGadget
2024-05-28 12:05 ` Patrick Steinhardt
2024-05-30 8:54 ` Xing Xin
2024-05-30 8:21 ` [PATCH v4 0/4] object checking related additions and fixes for bundles in fetches blanet via GitGitGadget
2024-05-30 8:21 ` [PATCH v4 1/4] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-05-30 8:21 ` [PATCH v4 2/4] unbundle: extend verify_bundle_flags to support fsck-objects Xing Xin via GitGitGadget
2024-06-06 12:06 ` Patrick Steinhardt
2024-06-11 6:46 ` Xing Xin
2024-05-30 8:21 ` [PATCH v4 3/4] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-05-30 8:21 ` [PATCH v4 4/4] unbundle: introduce option VERIFY_BUNDLE_FSCK_FOLLOW_FETCH Xing Xin via GitGitGadget
2024-06-06 12:06 ` Patrick Steinhardt
2024-06-11 6:46 ` Xing Xin
2024-06-11 6:42 ` [PATCH v5 0/4] object checking related additions and fixes for bundles in fetches blanet via GitGitGadget
2024-06-11 6:42 ` [PATCH v5 1/4] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-06-11 6:42 ` [PATCH v5 2/4] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-06-11 6:42 ` [PATCH v5 3/4] unbundle: extend options to support object verification Xing Xin via GitGitGadget
2024-06-11 9:11 ` Patrick Steinhardt
2024-06-11 12:47 ` Xing Xin
2024-06-11 6:42 ` [PATCH v5 4/4] unbundle: use VERIFY_BUNDLE_FSCK_FOLLOW_FETCH for fetches Xing Xin via GitGitGadget
2024-06-11 12:45 ` [PATCH v6 0/3] object checking related additions and fixes for bundles in fetches blanet via GitGitGadget
2024-06-11 12:45 ` [PATCH v6 1/3] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-06-11 19:08 ` Junio C Hamano
2024-06-17 13:53 ` Xing Xin
2024-06-11 12:45 ` [PATCH v6 2/3] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-06-11 19:20 ` Junio C Hamano
2024-06-11 12:45 ` [PATCH v6 3/3] unbundle: support object verification for fetches Xing Xin via GitGitGadget
2024-06-11 20:05 ` Junio C Hamano [this message]
2024-06-12 18:33 ` Xing Xin
2024-06-11 13:14 ` [PATCH v6 0/3] object checking related additions and fixes for bundles in fetches Patrick Steinhardt
2024-06-17 13:55 ` [PATCH v7 " blanet via GitGitGadget
2024-06-17 13:55 ` [PATCH v7 1/3] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-06-18 17:37 ` Junio C Hamano
2024-06-19 6:30 ` Xing Xin
2024-06-17 13:55 ` [PATCH v7 2/3] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-06-17 13:55 ` [PATCH v7 3/3] unbundle: extend object verification for fetches Xing Xin via GitGitGadget
2024-06-19 4:07 ` [PATCH v8 0/3] object checking related additions and fixes for bundles in fetches blanet via GitGitGadget
2024-06-19 4:07 ` [PATCH v8 1/3] bundle-uri: verify oid before writing refs Xing Xin via GitGitGadget
2024-06-19 4:07 ` [PATCH v8 2/3] fetch-pack: expose fsckObjects configuration logic Xing Xin via GitGitGadget
2024-06-19 4:07 ` [PATCH v8 3/3] unbundle: extend object verification for fetches Xing Xin via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsexjtfcg.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=bupt_xingxin@163.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=karthik.188@gmail.com \
--cc=ps@pks.im \
--cc=xingxin.xx@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).