{"thread":{"id":"62525","subject":"[PATCH 0/5] propagate fsck message severity for bundle fetch","startedAt":"2024-11-21T20:43:49Z","lastAt":"2024-12-04T02:39:07Z","messageCount":41,"participants":["Justin Tobler","Junio C Hamano","shejialuo","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"507841","messageId":"20241121204119.1440773-1-jltobler@gmail.com","threadId":"62525","inReplyTo":null,"subject":"[PATCH 0/5] propagate fsck message severity for bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:14Z","receivedAt":"2024-11-21T20:43:49Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nWith 63d903ff52 (unbundle: extend object verification for fetches,\n2024-06-19), fsck checks are now performed on fetched bundles depending\non `transfer.fsckObjects` and `fetch.fsckObjects` configuration. This\nworks, but provides no means to override the default fsck message\nseverity as is done for other git-fetch(1) operations. This series aims\nto propagate fsck message severity configuration to the underlying\ngit-index-pack(1) process executed on the bundle in a similar manner as\ndone with git-fetch-pack(1).\n\n - Patches 1 and 2 adapt the bundle subsystem to support additional\n options when performing `unbundle()`.\n\n - Patches 3 and 4 adapt the fetch-pack subsystem to expose a means to\n generate the message configuration arguments.\n\n - Patch 5 wires the newly generated fsck configuration options to the\n bundle options used when fetching from a bundle.\n\nThanks\n-Justin\n\nJustin Tobler (5):\n bundle: add bundle verification options type\n bundle: support fsck message configuration\n fetch-pack: introduce `fetch_pack_options`\n fetch-pack: expose `fetch_pack_config_cb()`\n transport: propagate fsck configuration during bundle fetch\n\n builtin/bundle.c | 2 +-\n bundle-uri.c | 13 ++++++++-----\n bundle.c | 17 +++++++++++------\n bundle.h | 15 ++++++++++-----\n fetch-pack.c | 20 +++++++++++---------\n fetch-pack.h | 12 ++++++++++++\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 11 +++++++++--\n 8 files changed, 69 insertions(+), 28 deletions(-)\n\n\nbase-commit: 4083a6f05206077a50af7658bedc17a94c54607d\n-- \n2.47.0\n\n"},{"id":"507842","messageId":"20241121204119.1440773-2-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH 1/5] bundle: add bundle verification options type","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:15Z","receivedAt":"2024-11-21T20:44:02Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Bundle verification performed as part of `unbundle()` is configurable\nvia providing `verify_bundle_flags`. This is done by invoking\n`verify_bundle()` and propagating the set flags. If the\n`VERIFY_BUNDLE_FSCK` flag is provided, the `fsck-objects` flag is\nspecified when invoking git-index-pack(1) to perform fsck checks on the\nobjects in the bundle.\n\nIntroduce a new type, `verify_bundle_opts`, and update `unbundle()` to\naccept this instead of `verify_bundle_flags` to perform the same\nverification configuration. In a subsequent commit, `verify_bundle_opts`\nwill be extended to support configuration of fsck message severity.\n\nSigned-off-by: Justin Tobler \n---\n builtin/bundle.c | 2 +-\n bundle-uri.c | 13 ++++++++-----\n bundle.c | 14 +++++++++-----\n bundle.h | 14 +++++++++-----\n transport.c | 6 ++++--\n 5 files changed, 31 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 127518c2a8..15ac75ab51 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -218,7 +218,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n \t\t\t _(\"Unbundling objects\"), NULL);\n \tret = !!unbundle(the_repository, &header, bundle_fd,\n-\t\t\t &extra_index_pack_args, 0) ||\n+\t\t\t &extra_index_pack_args, NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n \ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 0df66e2872..ed3afcaeb3 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -361,12 +361,16 @@ static int copy_uri_to_file(const char *filename, const char *uri)\n \n static int unbundle_from_file(struct repository *r, const char *file)\n {\n-\tint result = 0;\n-\tint bundle_fd;\n+\tstruct verify_bundle_opts opts = {\n+\t\t.flags = VERIFY_BUNDLE_QUIET |\n+\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)\n+\t};\n \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n-\tstruct string_list_item *refname;\n \tstruct strbuf bundle_ref = STRBUF_INIT;\n+\tstruct string_list_item *refname;\n \tsize_t bundle_prefix_len;\n+\tint result = 0;\n+\tint bundle_fd;\n \n \tbundle_fd = read_bundle_header(file, &header);\n \tif (bundle_fd < 0) {\n@@ -379,8 +383,7 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \t * a reachable ref pointing to the new tips, which will reach\n \t * the prerequisite commits.\n \t */\n-\tresult = unbundle(r, &header, bundle_fd, NULL,\n-\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n+\tresult = unbundle(r, &header, bundle_fd, NULL, &opts);\n \tif (result) {\n \t\tresult = 1;\n \t\tgoto cleanup;\ndiff --git a/bundle.c b/bundle.c\nindex 4773b51eb1..db17f50ee0 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -626,13 +626,17 @@ int create_bundle(struct repository *r, const char *path,\n \treturn ret;\n }\n \n-int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags)\n+int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n+\t struct strvec *extra_index_pack_args,\n+\t struct verify_bundle_opts *_opts)\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tstruct verify_bundle_opts opts = { 0 };\n \n-\tif (verify_bundle(r, header, flags))\n+\tif (_opts)\n+\t\topts = *_opts;\n+\n+\tif (verify_bundle(r, header, opts.flags))\n \t\treturn -1;\n \n \tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n@@ -641,7 +645,7 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \tif (header->filter.choice)\n \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n \n-\tif (flags & VERIFY_BUNDLE_FSCK)\n+\tif (opts.flags & VERIFY_BUNDLE_FSCK)\n \t\tstrvec_push(&ip.args, \"--fsck-objects\");\n \n \tif (extra_index_pack_args)\ndiff --git a/bundle.h b/bundle.h\nindex 5ccc9a061a..bddf44c267 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -39,6 +39,10 @@ enum verify_bundle_flags {\n int verify_bundle(struct repository *r, struct bundle_header *header,\n \t\t enum verify_bundle_flags flags);\n \n+struct verify_bundle_opts {\n+\tenum verify_bundle_flags flags;\n+};\n+\n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n@@ -49,12 +53,12 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n * \"extra_index_pack_args\" (if any) will be strvec_clear()'d for you.\n *\n- * Before unbundling, this method will call verify_bundle() with the\n- * given 'flags'.\n+ * Before unbundling, this method will call verify_bundle() with 'flags'\n+ * provided in 'opts'.\n */\n-int unbundle(struct repository *r, struct bundle_header *header,\n-\t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags);\n+int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n+\t struct strvec *extra_index_pack_args,\n+\t struct verify_bundle_opts *opts);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 47fda6a773..7e0ec4adc9 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -176,6 +176,8 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t\t int nr_heads UNUSED,\n \t\t\t\t struct ref **to_fetch UNUSED)\n {\n+\tstruct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?\n+\t\t\t\t\t\t\t VERIFY_BUNDLE_FSCK : 0 };\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n@@ -185,9 +187,9 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n+\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t &extra_index_pack_args,\n-\t\t fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);\n+\t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n-- \n2.47.0\n\n"},{"id":"507843","messageId":"20241121204119.1440773-3-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH 2/5] bundle: support fsck message configuration","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:16Z","receivedAt":"2024-11-21T20:44:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"The git-index-pack(1) spawned during `unbundle()` can be optionally\nconfigured with `--fsck-objects` to perform fsck checks on the bundle.\nThis does not propagate fsck message severity configuration though.\n\nExtend `verify_bundle_opts` to store this information and update\n`unbundle()` to configure the `--fsck-objects` option appropriately.\n\nSigned-off-by: Justin Tobler \n---\n bundle.c | 3 ++-\n bundle.h | 1 +\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex db17f50ee0..97b70e2e51 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -646,7 +646,8 @@ int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n \n \tif (opts.flags & VERIFY_BUNDLE_FSCK)\n-\t\tstrvec_push(&ip.args, \"--fsck-objects\");\n+\t\tstrvec_pushf(&ip.args, \"--fsck-objects%s\",\n+\t\t\t opts.fsck_msg_types ? opts.fsck_msg_types : \"\");\n \n \tif (extra_index_pack_args)\n \t\tstrvec_pushv(&ip.args, extra_index_pack_args->v);\ndiff --git a/bundle.h b/bundle.h\nindex bddf44c267..2a7b556f83 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -41,6 +41,7 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n \n struct verify_bundle_opts {\n \tenum verify_bundle_flags flags;\n+\tconst char *fsck_msg_types;\n };\n \n /**\n-- \n2.47.0\n\n"},{"id":"507844","messageId":"20241121204119.1440773-4-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH 3/5] fetch-pack: introduce `fetch_pack_options`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:17Z","receivedAt":"2024-11-21T20:44:07Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When `fetch_pack_config()` is invoked, fetch-pack configuration is\nparsed from the config. As part of this operation, fsck message severity\nconfiguration is assigned to the `fsck_msg_types` global variable. This\nis optionally used to configure the downstream git-index-pack(1) when\nthe `--strict` option is specified.\n\nIn a subsequent commit, the same fetch-pack fsck message configuration\nneeds to be reused. To facilitate this, introduce `fetch_pack_options`\nwhich gets written to during the `fetch_pack_config_cb()` instead of\ndirectly modifying the `fsck_msg_types` global.\n\nSigned-off-by: Justin Tobler \n---\n fetch-pack.c | 16 +++++++++-------\n fetch-pack.h | 8 ++++++++\n 2 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex fe1fb3c1b7..73309f8043 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -48,8 +48,8 @@ static int server_supports_filtering;\n static int advertise_sid;\n static struct shallow_lock shallow_lock;\n static const char *alternate_shallow_file;\n+static struct fetch_pack_options fetch_pack_options = FETCH_PACK_OPTIONS_INIT;\n static struct fsck_options fsck_options = FSCK_OPTIONS_MISSING_GITMODULES;\n-static struct strbuf fsck_msg_types = STRBUF_INIT;\n static struct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \n /* Remember to update object flag allocation in object.h */\n@@ -1017,7 +1017,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\tstrvec_push(&cmd.args, \"--fsck-objects\");\n \t\telse\n \t\t\tstrvec_pushf(&cmd.args, \"--strict%s\",\n-\t\t\t\t fsck_msg_types.buf);\n+\t\t\t\t fetch_pack_options.fsck_msg_types.buf);\n \t}\n \n \tif (index_pack_args) {\n@@ -1860,6 +1860,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n static int fetch_pack_config_cb(const char *var, const char *value,\n \t\t\t\tconst struct config_context *ctx, void *cb)\n {\n+\tstruct fetch_pack_options *opts = cb;\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n@@ -1867,8 +1868,8 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn 1;\n-\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n-\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tstrbuf_addf(&opts->fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\t opts->fsck_msg_types.len ? ',' : '=', path);\n \t\tfree(path);\n \t\treturn 0;\n \t}\n@@ -1877,8 +1878,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n \t\tif (is_valid_msg_type(msg_id, value))\n-\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n-\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n+\t\t\tstrbuf_addf(&opts->fsck_msg_types, \"%c%s=%s\",\n+\t\t\t\t opts->fsck_msg_types.len ? ',' : '=',\n+\t\t\t\t msg_id, value);\n \t\telse\n \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n \t\treturn 0;\n@@ -1904,7 +1906,7 @@ static void fetch_pack_config(void)\n \t\t}\n \t}\n \n-\tgit_config(fetch_pack_config_cb, NULL);\n+\tgit_config(fetch_pack_config_cb, &fetch_pack_options);\n }\n \n static void fetch_pack_setup(void)\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex b5c579cdae..8243b754ce 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -106,4 +106,12 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);\n */\n int fetch_pack_fsck_objects(void);\n \n+struct fetch_pack_options {\n+\tstruct strbuf fsck_msg_types;\n+};\n+\n+#define FETCH_PACK_OPTIONS_INIT { \\\n+\t.fsck_msg_types = STRBUF_INIT, \\\n+}\n+\n #endif\n-- \n2.47.0\n\n"},{"id":"507845","messageId":"20241121204119.1440773-5-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:18Z","receivedAt":"2024-11-21T20:44:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"During fetch-pack operations, git-index-pack(1) may be spawned and\nperform fsck checks. The message severity of these checks is\nconfigurable and propagated via appending it to the `--fsck-objects`\noption.\n\nWith `fetch_pack_config_cb()`, fsck configuration gets populated to a\n`fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate\nformatted fsck message configuration generation. In a subsequent commit,\nthis is used to wire message configuration to `unbundle()` during bundle\nfetches.\n\nSigned-off-by: Justin Tobler \n---\n fetch-pack.c | 4 ++--\n fetch-pack.h | 4 ++++\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 73309f8043..10b66795bc 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1857,8 +1857,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n-static int fetch_pack_config_cb(const char *var, const char *value,\n-\t\t\t\tconst struct config_context *ctx, void *cb)\n+int fetch_pack_config_cb(const char *var, const char *value,\n+\t\t\t const struct config_context *ctx, void *cb)\n {\n \tstruct fetch_pack_options *opts = cb;\n \tconst char *msg_id;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 8243b754ce..f35a75a3c5 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -1,6 +1,7 @@\n #ifndef FETCH_PACK_H\n #define FETCH_PACK_H\n \n+#include \"config.h\"\n #include \"string-list.h\"\n #include \"protocol.h\"\n #include \"list-objects-filter-options.h\"\n@@ -114,4 +115,7 @@ struct fetch_pack_options {\n \t.fsck_msg_types = STRBUF_INIT, \\\n }\n \n+int fetch_pack_config_cb(const char *var, const char *value,\n+\t\t\t const struct config_context *ctx, void *cb);\n+\n #endif\n-- \n2.47.0\n\n"},{"id":"507846","messageId":"20241121204119.1440773-6-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH 5/5] transport: propagate fsck configuration during bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-21T20:41:19Z","receivedAt":"2024-11-21T20:44:13Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When fetching directly from a bundle, fsck message severity\nconfiguration is not propagated to the underlying git-index-pack(1). It\nis only capable of enabling or disabling fsck checks entirely. This does\nnot align with the fsck behavior for fetches through git-fetch-pack(1).\n\nUse the message configuration from fetch-pack and wire it through to\n`unbundle()` to enable the same fsck configuration as done through\nfetch-pack.\n\nSigned-off-by: Justin Tobler \n---\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 5 +++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 7ceaa8194d..c69aa88eae 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -171,6 +171,13 @@ test_expect_success 'clone bundle with different fsckObjects configurations' '\n \n \ttest_must_fail git -c transfer.fsckObjects=true \\\n \t\tclone bundle-fsck/bad.bundle bundle-transfer-fsck 2>err &&\n+\ttest_grep \"missingEmail\" err &&\n+\n+\tgit -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=ignore \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-ignore &&\n+\n+\ttest_must_fail git -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=error \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-error 2>err &&\n \ttest_grep \"missingEmail\" err\n '\n \ndiff --git a/transport.c b/transport.c\nindex 7e0ec4adc9..a9e03c3964 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -178,6 +178,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n {\n \tstruct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?\n \t\t\t\t\t\t\t VERIFY_BUNDLE_FSCK : 0 };\n+\tstruct fetch_pack_options fetch_pack_options = FETCH_PACK_OPTIONS_INIT;\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n@@ -188,11 +189,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n \n+\tgit_config(fetch_pack_config_cb, &fetch_pack_options);\n+\topts.fsck_msg_types = fetch_pack_options.fsck_msg_types.buf;\n+\n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n+\tstrbuf_release(&fetch_pack_options.fsck_msg_types);\n \treturn ret;\n }\n \n-- \n2.47.0\n\n"},{"id":"507872","messageId":"xmqqy11ccbco.fsf@gitster.g","threadId":"62525","inReplyTo":"20241121204119.1440773-2-jltobler@gmail.com","subject":"Re: [PATCH 1/5] bundle: add bundle verification options type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T01:21:11Z","receivedAt":"2024-11-22T01:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 0df66e2872..ed3afcaeb3 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -361,12 +361,16 @@ static int copy_uri_to_file(const char *filename, const char *uri)\n> \n> static int unbundle_from_file(struct repository *r, const char *file)\n> {\n> -\tint result = 0;\n> -\tint bundle_fd;\n> +\tstruct verify_bundle_opts opts = {\n> +\t\t.flags = VERIFY_BUNDLE_QUIET |\n> +\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)\n> +\t};\n> \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n> -\tstruct string_list_item *refname;\n> \tstruct strbuf bundle_ref = STRBUF_INIT;\n> +\tstruct string_list_item *refname;\n> \tsize_t bundle_prefix_len;\n> +\tint result = 0;\n> +\tint bundle_fd;\n\nUnrelated changes to reorder the lines, without any justification\nworth describing in the proposed commit log message, distracts and\ndiscourages the reviewers from reading further on. I would avoid\nmaking such changes if I were doing this patch.\n\nThe _real_ change in the above hunk is that a new struct instance\n\"opts\" is defined, with its .flags member initialized based on what\nfetch_pack_fsck_object() says. That helper function requires us to\nbe in a repository, but because you must have a repository to\nunbundle into, that call is safe.\n\n> @@ -379,8 +383,7 @@ static int unbundle_from_file(struct repository *r, const char *file)\n> \t * a reachable ref pointing to the new tips, which will reach\n> \t * the prerequisite commits.\n> \t */\n> -\tresult = unbundle(r, &header, bundle_fd, NULL,\n> -\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n> +\tresult = unbundle(r, &header, bundle_fd, NULL, &opts);\n\nWe can see that .flags in the new structure gets the same value we\nused to pass in the original, which is good.\n\n> diff --git a/bundle.c b/bundle.c\n> index 4773b51eb1..db17f50ee0 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -626,13 +626,17 @@ int create_bundle(struct repository *r, const char *path,\n> \treturn ret;\n> }\n> \n> -int unbundle(struct repository *r, struct bundle_header *header,\n> -\t int bundle_fd, struct strvec *extra_index_pack_args,\n> -\t enum verify_bundle_flags flags)\n> +int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n> +\t struct strvec *extra_index_pack_args,\n> +\t struct verify_bundle_opts *_opts)\n\nAgain, unrelated rewrapping of lines distracts and discourages the\nreviewers from reading further on. It looked as if the patch is\nadding an extra parameter, until I read it again.\n\nThe real change here is that the enum is replaced with a struct that\nhas the same enum as one of its members, which is good.\n\nName the external-facing one (like this new parameter) _without_\nfunnies, and call it straight \"opts\". The internal stand-in object\nyou create below can use funny convention but using \"_\" as prefix is\nusually for system stuff (and the language standard forbids it, even\nthough people often do so in practice, from programs).\n\n> {\n> \tstruct child_process ip = CHILD_PROCESS_INIT;\n> +\tstruct verify_bundle_opts opts = { 0 };\n> \n> -\tif (verify_bundle(r, header, flags))\n> +\tif (_opts)\n> +\t\topts = *_opts;\n> +\n> +\tif (verify_bundle(r, header, opts.flags))\n> \t\treturn -1;\n\nThis is an arrangement that looks strange, especially at this stage\nof the series without reading the rest. If verify_bundle() takes\nthe enum and not &opts, there is no need for stand-in opts struct.\nYou can have a local enum \"flags\" that is initialized to 0 and only\nwhen parameter \"opts\" is not NULL, assign opts->flags to it and use\nit throughout the rest of the function. Reviewers will be left\nconfused wondering why the patch does this in an unnecessarily more\ncomplex way by using an extra structure instance.\n\nUntil you start needing other fields of opts in the function,\nperhaps in a later step, that is.\n\n> @@ -641,7 +645,7 @@ int unbundle(struct repository *r, struct bundle_header *header,\n> \tif (header->filter.choice)\n> \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n> \n> -\tif (flags & VERIFY_BUNDLE_FSCK)\n> +\tif (opts.flags & VERIFY_BUNDLE_FSCK)\n> \t\tstrvec_push(&ip.args, \"--fsck-objects\");\n\nAnd this is a fallout of the above \"strange\" arrangement.\n\n> {\n> +\tstruct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?\n> +\t\t\t\t\t\t\t VERIFY_BUNDLE_FSCK : 0 };\n\n\tstruct verify_bundle_opts opts = {\n\t\t.flags = fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0,\n\t};\n\nto avoid overly long lines, and prepare for a future you add more\nmembers to the structure (the trailing comma helps).\n\n"},{"id":"507874","messageId":"xmqqttc0cawy.fsf@gitster.g","threadId":"62525","inReplyTo":"20241121204119.1440773-3-jltobler@gmail.com","subject":"Re: [PATCH 2/5] bundle: support fsck message configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T01:30:37Z","receivedAt":"2024-11-22T01:30:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> The git-index-pack(1) spawned during `unbundle()` can be optionally\n> configured with `--fsck-objects` to perform fsck checks on the bundle.\n> This does not propagate fsck message severity configuration though.\n>\n> Extend `verify_bundle_opts` to store this information and update\n> `unbundle()` to configure the `--fsck-objects` option appropriately.\n>\n> Signed-off-by: Justin Tobler \n> ---\n> bundle.c | 3 ++-\n> bundle.h | 1 +\n> 2 files changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/bundle.c b/bundle.c\n> index db17f50ee0..97b70e2e51 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -646,7 +646,8 @@ int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n> \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n> \n> \tif (opts.flags & VERIFY_BUNDLE_FSCK)\n> -\t\tstrvec_push(&ip.args, \"--fsck-objects\");\n> +\t\tstrvec_pushf(&ip.args, \"--fsck-objects%s\",\n> +\t\t\t opts.fsck_msg_types ? opts.fsck_msg_types : \"\");\n\nOK, having %s immediately after --option-name means that anybody who\nis adding anything to fsck_msg_types is responsible for starting it\nwith an \"=\" equals sign, but that is in line with how existing code\ndoes, e.g. receive-pack drives unpack-objcts/index-pack with the\n\"--strict%s\" option with a potential value for fsck_msg_types).\n\n> diff --git a/bundle.h b/bundle.h\n> index bddf44c267..2a7b556f83 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -41,6 +41,7 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n> \n> struct verify_bundle_opts {\n> \tenum verify_bundle_flags flags;\n> +\tconst char *fsck_msg_types;\n> };\n> \n> /**\n"},{"id":"507875","messageId":"xmqqjzcwca74.fsf@gitster.g","threadId":"62525","inReplyTo":"20241121204119.1440773-4-jltobler@gmail.com","subject":"Re: [PATCH 3/5] fetch-pack: introduce `fetch_pack_options`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T01:46:07Z","receivedAt":"2024-11-22T01:46:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> When `fetch_pack_config()` is invoked, fetch-pack configuration is\n> parsed from the config. As part of this operation, fsck message severity\n> configuration is assigned to the `fsck_msg_types` global variable. This\n> is optionally used to configure the downstream git-index-pack(1) when\n> the `--strict` option is specified.\n>\n> In a subsequent commit, the same fetch-pack fsck message configuration\n> needs to be reused. To facilitate this, introduce `fetch_pack_options`\n> which gets written to during the `fetch_pack_config_cb()` instead of\n> directly modifying the `fsck_msg_types` global.\n\nIt is unclear how it facilitates to replace one global with another\nglobal that has the data that was previously global as one of its\nmembers. With the above I was somehow expecting that the option\nstruct instance is allocated on the stack of a function common to\nboth callers of the configuration reader (i.e. fetch_pack_config())\nas well as the configuration user (i.e. get_pack()). If we were to\nallow the latter to keep accessing the global (which is perfectly\nfine), wouldn't it be sufficient for the purpose of this series\n(which I am imagining wants to call fetch_pack_config() from the\nsideways and grab what came from the configuration) to just pass the\nfsck_msg_types strbuf through the call chain of the reaading side?\n\nThat is,\n\n - fetch_pack_config()'s current callers pass the address of\n fsck_msg_types to a new parameter.\n\n - fetch_pack_config() passes that new parameter when calling\n git_config();\n\n - fetch_pack_config_cb() uses the cb parameter and stuff its\n findings there;\n\n - a third-party caller calls fetch_pack_config() with its own\n fsck_msg_types instance (presumably in this series, it would be\n the opts.fsck_msg_types member introduced earlier in the bundle\n code).\n\nor something like that?\n\nSo, the reason for existence of the shell around the fsck_msg_types\nneeds to be explained. It is perfectly fine to say \"we'll add THIS\nTHING in a later step\", if that were the case, but a reviewer tends\nto start reading from the front, so the presentation order matters.\n\nLeaving many questions unsolved tangling may be a good way to keep\nreaders engaged when writing a mystery novel, but not a patch\nseries. Having to keep too many things in head, especially when\nmany of them are not explained well (hence raises \"why should I keep\nthese in my head?\" question), is another distraction and discourages\nthe reviewers from reading further on.\n\nAssuming that the shell structure is necessary around it, the code\nchanges in this patch looks sensible to me.\n\nThanks.\n"},{"id":"507876","messageId":"xmqqa5dsc9nq.fsf@gitster.g","threadId":"62525","inReplyTo":"20241121204119.1440773-5-jltobler@gmail.com","subject":"Re: [PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T01:57:45Z","receivedAt":"2024-11-22T01:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> With `fetch_pack_config_cb()`, fsck configuration gets populated to a\n> `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate\n> formatted fsck message configuration generation. In a subsequent commit,\n> this is used to wire message configuration to `unbundle()` during bundle\n> fetches.\n\nThis is generally going in the right direction, but this particular\niteration is highly disappointing for two reasons.\n\n - The callback calls git_default_config() at end. Other callers\n may not want it happen. Think of the reason why a new caller may\n want to use this callback (see the next item).\n\n - fetch_pack_config_cb() was perfectly good name back when it was\n hidden inside fetch-pack.c, as a private internal implementation\n detail, EVEN THOUGH it did not give its callers everything that\n tries to configure the behaviour of fetch-pack. It ONLY is about\n how fsck behaviour is affected. It must be renamed so that any\n new caller can realize that it is configuring fsck checking\n machinery and not general fetch-pack features.\n\nSo, I would suggest making at least two changes.\n\n - rename it to a more sensible name that includes \"fsck\" somewhere\n (as it is about \"fetch.fsck.*\" configuration variables, \"fetch\"\n should also stay in the name). Let's tentatively call it foo().\n\n - stop calling git_default_config() from foo(). Instead, return\n some special value foo() does not currently return, let's say -1\n to signal that the key was something foo() was not interested in,\n and write a thin replacement helper\n\n static int fetch_pack_config_cb(...)\n {\n\tint st = foo(...);\n\tif (st < 0)\n\t return git_default_config(var, value, ctx, cb);\n\treturn st;\n }\n\n and call that from fetch_pack_config().\n\nNo, \"foo()\" has neither \"fetch\" or \"fsck\" in it; I am not suggesting\nto use that as the final name ;-).\n\nThanks.\n"},{"id":"507877","messageId":"xmqq5xogc9k3.fsf@gitster.g","threadId":"62525","inReplyTo":"20241121204119.1440773-6-jltobler@gmail.com","subject":"Re: [PATCH 5/5] transport: propagate fsck configuration during bundle fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T01:59:56Z","receivedAt":"2024-11-22T01:59:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> When fetching directly from a bundle, fsck message severity\n> configuration is not propagated to the underlying git-index-pack(1). It\n> is only capable of enabling or disabling fsck checks entirely. This does\n> not align with the fsck behavior for fetches through git-fetch-pack(1).\n>\n> Use the message configuration from fetch-pack and wire it through to\n> `unbundle()` to enable the same fsck configuration as done through\n> fetch-pack.\n>\n> Signed-off-by: Justin Tobler \n> ---\n> t/t5607-clone-bundle.sh | 7 +++++++\n> transport.c | 5 +++++\n> 2 files changed, 12 insertions(+)\n\nNicely done.\n\n>\n> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\n> index 7ceaa8194d..c69aa88eae 100755\n> --- a/t/t5607-clone-bundle.sh\n> +++ b/t/t5607-clone-bundle.sh\n> @@ -171,6 +171,13 @@ test_expect_success 'clone bundle with different fsckObjects configurations' '\n> \n> \ttest_must_fail git -c transfer.fsckObjects=true \\\n> \t\tclone bundle-fsck/bad.bundle bundle-transfer-fsck 2>err &&\n> +\ttest_grep \"missingEmail\" err &&\n> +\n> +\tgit -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=ignore \\\n> +\t\tclone bundle-fsck/bad.bundle bundle-fsck-ignore &&\n> +\n> +\ttest_must_fail git -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=error \\\n> +\t\tclone bundle-fsck/bad.bundle bundle-fsck-error 2>err &&\n> \ttest_grep \"missingEmail\" err\n> '\n> \n> diff --git a/transport.c b/transport.c\n> index 7e0ec4adc9..a9e03c3964 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -178,6 +178,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n> {\n> \tstruct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?\n> \t\t\t\t\t\t\t VERIFY_BUNDLE_FSCK : 0 };\n> +\tstruct fetch_pack_options fetch_pack_options = FETCH_PACK_OPTIONS_INIT;\n> \tstruct bundle_transport_data *data = transport->data;\n> \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n> \tint ret;\n> @@ -188,11 +189,15 @@ static int fetch_refs_from_bundle(struct transport *transport,\n> \tif (!data->get_refs_from_bundle_called)\n> \t\tget_refs_from_bundle_inner(transport);\n> \n> +\tgit_config(fetch_pack_config_cb, &fetch_pack_options);\n> +\topts.fsck_msg_types = fetch_pack_options.fsck_msg_types.buf;\n> +\n> \tret = unbundle(the_repository, &data->header, data->fd,\n> \t\t &extra_index_pack_args, &opts);\n> \ttransport->hash_algo = data->header.hash_algo;\n> \n> \tstrvec_clear(&extra_index_pack_args);\n> +\tstrbuf_release(&fetch_pack_options.fsck_msg_types);\n> \treturn ret;\n> }\n"},{"id":"507881","messageId":"xmqq5xogaq2u.fsf@gitster.g","threadId":"62525","inReplyTo":"xmqqjzcwca74.fsf@gitster.g","subject":"Re: [PATCH 3/5] fetch-pack: introduce `fetch_pack_options`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T03:46:01Z","receivedAt":"2024-11-22T03:46:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Assuming that the shell structure is necessary around it, the code\n> changes in this patch looks sensible to me.\n\nAh, another thing. It would make more sense to do\n\n good_example(args, struct foo *opt)\n {\n\tstruct foo opt_fallback = { ... init ... };\n\n\tif (!opt)\n\t\topt = &opt_fallback;\n\t...\n\tuse opt->foo and opt->bar\n }\n\ninstead of what the patch did with structure assignment, i.e.\n\n bad_example(args, struct foo *opt_)\n {\n\tstruct foo opt = { ... init ... };\n\n\tif (opt_)\n\t\topt = *opt_;\n\t...\n\tuse opt.foo and opt.bar\n }\n\nbecause the latter, via structure assignment, always raises \"who\nowns this piece of data?\" question once you start adding more\ncomplex things in the \"struct foo\", like a strbuf that holds the\nfsck configuration data.\n"},{"id":"507929","messageId":"o22ukwk2b6pewcz4t4coplhu3nnygrlescyko6hj33ak2yynx2@zkq4oa3pnguw","threadId":"62525","inReplyTo":"xmqqy11ccbco.fsf@gitster.g","subject":"Re: [PATCH 1/5] bundle: add bundle verification options type","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-22T15:22:35Z","receivedAt":"2024-11-22T15:24:29Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/22 10:21AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > diff --git a/bundle-uri.c b/bundle-uri.c\n> > index 0df66e2872..ed3afcaeb3 100644\n> > --- a/bundle-uri.c\n> > +++ b/bundle-uri.c\n> > @@ -361,12 +361,16 @@ static int copy_uri_to_file(const char *filename, const char *uri)\n> > \n> > static int unbundle_from_file(struct repository *r, const char *file)\n> > {\n> > -\tint result = 0;\n> > -\tint bundle_fd;\n> > +\tstruct verify_bundle_opts opts = {\n> > +\t\t.flags = VERIFY_BUNDLE_QUIET |\n> > +\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)\n> > +\t};\n> > \tstruct bundle_header header = BUNDLE_HEADER_INIT;\n> > -\tstruct string_list_item *refname;\n> > \tstruct strbuf bundle_ref = STRBUF_INIT;\n> > +\tstruct string_list_item *refname;\n> > \tsize_t bundle_prefix_len;\n> > +\tint result = 0;\n> > +\tint bundle_fd;\n> \n> Unrelated changes to reorder the lines, without any justification\n> worth describing in the proposed commit log message, distracts and\n> discourages the reviewers from reading further on. I would avoid\n> making such changes if I were doing this patch.\n\nThanks for the feedback. I'll revert the unnecessary changes in the next\nversion and avoid doing this in the future.\n\n> > -int unbundle(struct repository *r, struct bundle_header *header,\n> > -\t int bundle_fd, struct strvec *extra_index_pack_args,\n> > -\t enum verify_bundle_flags flags)\n> > +int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,\n> > +\t struct strvec *extra_index_pack_args,\n> > +\t struct verify_bundle_opts *_opts)\n> \n> Again, unrelated rewrapping of lines distracts and discourages the\n> reviewers from reading further on. It looked as if the patch is\n> adding an extra parameter, until I read it again.\n> \n> The real change here is that the enum is replaced with a struct that\n> has the same enum as one of its members, which is good.\n> \n> Name the external-facing one (like this new parameter) _without_\n> funnies, and call it straight \"opts\". The internal stand-in object\n> you create below can use funny convention but using \"_\" as prefix is\n> usually for system stuff (and the language standard forbids it, even\n> though people often do so in practice, from programs).\n\nGood to know, I saw this pattern used in the reftable library. I'll\nupdate per your suggestion in the next version.\n\n> > {\n> > \tstruct child_process ip = CHILD_PROCESS_INIT;\n> > +\tstruct verify_bundle_opts opts = { 0 };\n> > \n> > -\tif (verify_bundle(r, header, flags))\n> > +\tif (_opts)\n> > +\t\topts = *_opts;\n> > +\n> > +\tif (verify_bundle(r, header, opts.flags))\n> > \t\treturn -1;\n> \n> This is an arrangement that looks strange, especially at this stage\n> of the series without reading the rest. If verify_bundle() takes\n> the enum and not &opts, there is no need for stand-in opts struct.\n> You can have a local enum \"flags\" that is initialized to 0 and only\n> when parameter \"opts\" is not NULL, assign opts->flags to it and use\n> it throughout the rest of the function. Reviewers will be left\n> confused wondering why the patch does this in an unnecessarily more\n> complex way by using an extra structure instance.\n\nGood point, at this point in the series its not obvious why the added\ncompexity is good or worth it. I'll defer making this change to the\nfollowing patch in the next version.\n\n> \n> Until you start needing other fields of opts in the function,\n> perhaps in a later step, that is.\n\nYa, this is its intent. Having a default options to fallback to is more\nuseful once there are other fields present.\n\n-Justin\n"},{"id":"507937","messageId":"4icigeb6n75pbxqmpp3uynooef7dff2f73lxmrjjmppuz7hibo@bwb3tkffnqop","threadId":"62525","inReplyTo":"xmqqttc0cawy.fsf@gitster.g","subject":"Re: [PATCH 2/5] bundle: support fsck message configuration","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-22T15:44:33Z","receivedAt":"2024-11-22T15:46:30Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/22 10:30AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > \tif (opts.flags & VERIFY_BUNDLE_FSCK)\n> > -\t\tstrvec_push(&ip.args, \"--fsck-objects\");\n> > +\t\tstrvec_pushf(&ip.args, \"--fsck-objects%s\",\n> > +\t\t\t opts.fsck_msg_types ? opts.fsck_msg_types : \"\");\n> \n> OK, having %s immediately after --option-name means that anybody who\n> is adding anything to fsck_msg_types is responsible for starting it\n> with an \"=\" equals sign, but that is in line with how existing code\n> does, e.g. receive-pack drives unpack-objcts/index-pack with the\n> \"--strict%s\" option with a potential value for fsck_msg_types).\n\nI was considering making it the responsibility of the `fsck_msg_types`\nconsumer to conditionally preprend the '='. This would mean that\nfetch-pack, in the later patch, should also be updated to not\nautomatically prepend '=' when parsing the config and be adjusted\naccordingly. This does feel more intuitive to me, but I've not sure it\nwarrants the added fallout and complexity.\n\nIn the next version I'll add a comment to the type definition indicating\nthat any value for `fsck_msg_types` other than NULL, is expected to be\nprefixed with '='. I am open to changing how this works altogether\nthough if we think that is best. :)\n\n-Justin\n"},{"id":"507945","messageId":"Z0C1PxdltxDm8pyA@ArchLinux","threadId":"62525","inReplyTo":"20241121204119.1440773-5-jltobler@gmail.com","subject":"Re: [PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-11-22T16:45:51Z","receivedAt":"2024-11-22T16:45:41Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Nov 21, 2024 at 02:41:18PM -0600, Justin Tobler wrote:\n> During fetch-pack operations, git-index-pack(1) may be spawned and\n> perform fsck checks. The message severity of these checks is\n> configurable and propagated via appending it to the `--fsck-objects`\n> option.\n> \n> With `fetch_pack_config_cb()`, fsck configuration gets populated to a\n> `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate\n> formatted fsck message configuration generation. In a subsequent commit,\n> this is used to wire message configuration to `unbundle()` during bundle\n> fetches.\n> \n\nIn my perspective, we may not separate [PATCH 4/5] and [PATCH 5/5].\nShould the reason why we want to expose `fetch_pack_config_cb` is that\nwe need to propagate the fsck severity to `unbundle`? Without the\ninformation of the last patch, we cannot know any detail thing. So, they\nare highly relevant.\n\nHowever, from the comment of the Junio, there are a lot of things need\nto be changed. So, just a comment.\n\nThanks,\nJialuo\n"},{"id":"507947","messageId":"whh4tdhqmsjhfwsdkxjigw3j4ef33r4kpfika6azuonywil4eh@uf4qleljqxkm","threadId":"62525","inReplyTo":"xmqqjzcwca74.fsf@gitster.g","subject":"Re: [PATCH 3/5] fetch-pack: introduce `fetch_pack_options`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-22T17:31:16Z","receivedAt":"2024-11-22T17:33:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/22 10:46AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > When `fetch_pack_config()` is invoked, fetch-pack configuration is\n> > parsed from the config. As part of this operation, fsck message severity\n> > configuration is assigned to the `fsck_msg_types` global variable. This\n> > is optionally used to configure the downstream git-index-pack(1) when\n> > the `--strict` option is specified.\n> >\n> > In a subsequent commit, the same fetch-pack fsck message configuration\n> > needs to be reused. To facilitate this, introduce `fetch_pack_options`\n> > which gets written to during the `fetch_pack_config_cb()` instead of\n> > directly modifying the `fsck_msg_types` global.\n> \n> It is unclear how it facilitates to replace one global with another\n> global that has the data that was previously global as one of its\n> members. With the above I was somehow expecting that the option\n> struct instance is allocated on the stack of a function common to\n> both callers of the configuration reader (i.e. fetch_pack_config())\n> as well as the configuration user (i.e. get_pack()). If we were to\n> allow the latter to keep accessing the global (which is perfectly\n> fine), wouldn't it be sufficient for the purpose of this series\n> (which I am imagining wants to call fetch_pack_config() from the\n> sideways and grab what came from the configuration) to just pass the\n> fsck_msg_types strbuf through the call chain of the reaading side?\n\nFor the purposes of this series, the addition of the\n`fetch_pack_options` structure as a wrapper around `fsck_msg_types` is\nnot needed. As mentioned, it would be sufficient to just pass the\n`strbuf` directly and have it modified by `fetch_pack_config_cb()`.\n\nThe original intent of providing the shell structure was to allow for\nmore easy extension of the fetch-pack config parsing in the future, but\nit doesn't probably make much sense to do it now and its purpose wasn't\nexplained.\n\nIn the next version I'll drop the use of shell structure in favor of\npassing an instance of `strbuf` directly.\n\n-Justin\n"},{"id":"507948","messageId":"j36eguzz7vgd7j32csj4erjzfh6xi7zw7eijxuqesr3lp3cjlz@pk4lcbwi6bbx","threadId":"62525","inReplyTo":"xmqqa5dsc9nq.fsf@gitster.g","subject":"Re: [PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-22T17:41:06Z","receivedAt":"2024-11-22T17:42:58Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/22 10:57AM, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > With `fetch_pack_config_cb()`, fsck configuration gets populated to a\n> > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate\n> > formatted fsck message configuration generation. In a subsequent commit,\n> > this is used to wire message configuration to `unbundle()` during bundle\n> > fetches.\n> \n> This is generally going in the right direction, but this particular\n> iteration is highly disappointing for two reasons.\n> \n> - The callback calls git_default_config() at end. Other callers\n> may not want it happen. Think of the reason why a new caller may\n> want to use this callback (see the next item).\n> \n> - fetch_pack_config_cb() was perfectly good name back when it was\n> hidden inside fetch-pack.c, as a private internal implementation\n> detail, EVEN THOUGH it did not give its callers everything that\n> tries to configure the behaviour of fetch-pack. It ONLY is about\n> how fsck behaviour is affected. It must be renamed so that any\n> new caller can realize that it is configuring fsck checking\n> machinery and not general fetch-pack features.\n> \n> So, I would suggest making at least two changes.\n> \n> - rename it to a more sensible name that includes \"fsck\" somewhere\n> (as it is about \"fetch.fsck.*\" configuration variables, \"fetch\"\n> should also stay in the name). Let's tentatively call it foo().\n> \n> - stop calling git_default_config() from foo(). Instead, return\n> some special value foo() does not currently return, let's say -1\n> to signal that the key was something foo() was not interested in,\n> and write a thin replacement helper\n> \n> static int fetch_pack_config_cb(...)\n> {\n> \tint st = foo(...);\n> \tif (st < 0)\n> \t return git_default_config(var, value, ctx, cb);\n> \treturn st;\n> }\n> \n> and call that from fetch_pack_config().\n> \n> No, \"foo()\" has neither \"fetch\" or \"fsck\" in it; I am not suggesting\n> to use that as the final name ;-).\n> \n> Thanks.\n\nThanks for the suggestions. I'll factor out the fsck configuration bit\nas suggested and name it something like \"fetch_pack_fsck_config()\". The\nnew name should be more clear and this change will also ensure only the\ndesired fsck configuration is being parsed which makes more sense. :)\n\n-Justin\n"},{"id":"508002","messageId":"xmqqed3085d9.fsf@gitster.g","threadId":"62525","inReplyTo":"4icigeb6n75pbxqmpp3uynooef7dff2f73lxmrjjmppuz7hibo@bwb3tkffnqop","subject":"Re: [PATCH 2/5] bundle: support fsck message configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-25T01:33:06Z","receivedAt":"2024-11-25T01:33:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> I was considering making it the responsibility of the `fsck_msg_types`\n> consumer to conditionally preprend the '='.\n\nIn my initial reading, I too thought it might be logically more\nclear, but I changed my mind and the patch posted as is is fine\n(and that last part of the sentence is what I wanted to say).\n\nBecause fsck_msg_types starts as an empty strbuf and accumulates\nelements one at a time, each time we add something to it, we'd need\nto check if we are adding the first element (in which case we do not\nwant to terminate the existing list with \",\") or we already have\nsomething in there (in which case we do want to add \",\" before our\nnew element). Because we are doing the check anyway, instead of\nsaying \"ah, this is the first one, so let's not add ','\", saying\n\"the first one? we need '='\" is not too bad.\n\nAnd the consuming side would not have to have a conditional \"if the\nstring is empty, do nothing, otherwise add '=' and then the string\".\nThe consumers can just \"concatenate the string, which is possibly\nempty, after the option\".\n\nSo in the end, the complexity for the producer is the same, and the\nconsumer becomes much simpler.\n\nAnd this exactly pattern (which I personally find a bit ugly) is\nused by receive-pack to drive unpack-objects and index-pack, which\nmakes it doubly OK to use it.\n\nAdding a comment to describe what is expected in the variable is\nindeed very much appreciated, though.\n\nThanks.\n\n"},{"id":"508166","messageId":"Z0WQC7mFvT4pKn0n@pks.im","threadId":"62525","inReplyTo":"20241121204119.1440773-2-jltobler@gmail.com","subject":"Re: [PATCH 1/5] bundle: add bundle verification options type","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-26T09:08:32Z","receivedAt":"2024-11-26T09:08:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 21, 2024 at 02:41:15PM -0600, Justin Tobler wrote:\n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 0df66e2872..ed3afcaeb3 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -361,12 +361,16 @@ static int copy_uri_to_file(const char *filename, const char *uri)\n> \n> static int unbundle_from_file(struct repository *r, const char *file)\n> {\n> -\tint result = 0;\n> -\tint bundle_fd;\n> +\tstruct verify_bundle_opts opts = {\n> +\t\t.flags = VERIFY_BUNDLE_QUIET |\n> +\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)\n\nThis is missing its trailing comma.\n\n> diff --git a/bundle.h b/bundle.h\n> index 5ccc9a061a..bddf44c267 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -39,6 +39,10 @@ enum verify_bundle_flags {\n> int verify_bundle(struct repository *r, struct bundle_header *header,\n> \t\t enum verify_bundle_flags flags);\n\nCurious. I would have expected that `verify_bundle()` should receive the\nfull `verify_bundle_opts` because these are so similarly named. Like\nthis we have the weird situation where `unbundle()` seemingly receives\nthe options that are intended for `verify_bundle()`, but we never pass\nthem through in the first place.\n\n> +struct verify_bundle_opts {\n> +\tenum verify_bundle_flags flags;\n> +};\n> +\n> /**\n> * Unbundle after reading the header with read_bundle_header().\n> *\n> diff --git a/transport.c b/transport.c\n> index 47fda6a773..7e0ec4adc9 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -176,6 +176,8 @@ static int fetch_refs_from_bundle(struct transport *transport,\n> \t\t\t\t int nr_heads UNUSED,\n> \t\t\t\t struct ref **to_fetch UNUSED)\n> {\n> +\tstruct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?\n> +\t\t\t\t\t\t\t VERIFY_BUNDLE_FSCK : 0 };\n\nThis is weirdly formatted and should rather follow what you have done\nfurther up in `unbundle_from_file()`.\n\nPatrick\n"},{"id":"508187","messageId":"hg6gq4izm3jmmrmh5g6fy7ojxbgxc35y33gvm2blntfmto2hd2@ecd546locmbg","threadId":"62525","inReplyTo":"Z0WQC7mFvT4pKn0n@pks.im","subject":"Re: [PATCH 1/5] bundle: add bundle verification options type","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-26T15:59:20Z","receivedAt":"2024-11-26T16:01:19Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/26 10:08AM, Patrick Steinhardt wrote:\n> On Thu, Nov 21, 2024 at 02:41:15PM -0600, Justin Tobler wrote:\n> > diff --git a/bundle.h b/bundle.h\n> > index 5ccc9a061a..bddf44c267 100644\n> > --- a/bundle.h\n> > +++ b/bundle.h\n> > @@ -39,6 +39,10 @@ enum verify_bundle_flags {\n> > int verify_bundle(struct repository *r, struct bundle_header *header,\n> > \t\t enum verify_bundle_flags flags);\n> \n> Curious. I would have expected that `verify_bundle()` should receive the\n> full `verify_bundle_opts` because these are so similarly named. Like\n> this we have the weird situation where `unbundle()` seemingly receives\n> the options that are intended for `verify_bundle()`, but we never pass\n> them through in the first place.\n\nThe `verify_bundle_opts` type should probably be renamed to\n`unbundle_opts`. As you mentioned, these options are not used directly\nby `verify_bundle()` as the current naming would suggest which is\nconfusing. Really the intent is allow further configuration of\n`unbundle()`. This is evident in the next patch where the options are\nextended to further configure the git-index-pack(1) spawned through\n`unbundle()`. I'll update the naming in the next version.\n\n-Justin\n"},{"id":"508202","messageId":"20241127005707.319881-1-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241121204119.1440773-1-jltobler@gmail.com","subject":"[PATCH v2 0/4] propagate fsck message severity for bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T00:57:03Z","receivedAt":"2024-11-27T00:59:52Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nWith 63d903ff52 (unbundle: extend object verification for fetches,\n2024-06-19), fsck checks are now performed on fetched bundles depending\non `transfer.fsckObjects` and `fetch.fsckObjects` configuration. This\nworks, but provides no means to override the default fsck message\nseverity as is done for other git-fetch(1) operations. This series aims\nto propagate fsck message severity configuration to the underlying\ngit-index-pack(1) process executed on the bundle in a similar manner as\ndone with git-fetch-pack(1).\n\n - Patches 1 and 2 adapt the bundle subsystem to support additional\n options when performing `unbundle()`.\n\n - Patch 3 adapts the fetch-pack subsystem to expose a means to\n generate the message configuration arguments.\n\n - Patch 4 wires the newly generated fsck configuration options to the\n bundle options used when fetching from a bundle.\n\nChanges in V2:\n\n - Reverted unnecessary line formatted changes.\n\n - Simplified fallback logic for default `unbundle()` options.\n\n - Dropped unncessary shell structure in the third patch to keep the\n patch more straightforward.\n\n - Instead of exposing `fetch_pack_config_cb()` directly, the fetch\n pack fsck config parsing logic is split and exposed. This also\n addresses the issue of unwanted default Git config parsing occuring.\n Consequently the previous third and fourth patch were condensed into\n a single patch.\n\n - Improved explainations in the patches, naming, and various style\n fixes.\n\nDue to all rearranging in this version, I opted not to include a\nrange-diff.\n\nThanks\n-Justin\n\nJustin Tobler (4):\n bundle: add bundle verification options type\n bundle: support fsck message configuration\n fetch-pack: split out fsck config parsing\n transport: propagate fsck configuration during bundle fetch\n\n builtin/bundle.c | 2 +-\n bundle-uri.c | 7 +++++--\n bundle.c | 13 +++++++++----\n bundle.h | 17 ++++++++++++++---\n fetch-pack.c | 24 +++++++++++++++++-------\n fetch-pack.h | 3 +++\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 26 ++++++++++++++++++++++++--\n 8 files changed, 80 insertions(+), 19 deletions(-)\n\n\nbase-commit: 4083a6f05206077a50af7658bedc17a94c54607d\n-- \n2.47.0\n\n"},{"id":"508203","messageId":"20241127005707.319881-2-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127005707.319881-1-jltobler@gmail.com","subject":"[PATCH v2 1/4] bundle: add bundle verification options type","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T00:57:04Z","receivedAt":"2024-11-27T01:00:01Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When `unbundle()` is invoked, fsck verification may be configured by\npassing the `VERIFY_BUNDLE_FSCK` flag. This mechanism allows fsck checks\non the bundle to be enabled or disabled entirely. To facilitate more\nfine-grained fsck configuration, additional context must be provided to\n`unbundle()`.\n\nIntroduce the `unbundle_opts` type, which wraps the existing\n`verify_bundle_flags`, to facilitate future extension of `unbundle()`\nconfiguration. Also update `unbundle()` and its call sites to accept\nthis new options type instead of the flags directly. The end behavior is\nfunctionally the same, but allows for the set of configurable options to\nbe extended. This is leveraged in a subsequent commit to enable fsck\nmessage severity configuration.\n\nSigned-off-by: Justin Tobler \n---\n builtin/bundle.c | 2 +-\n bundle-uri.c | 7 +++++--\n bundle.c | 6 +++++-\n bundle.h | 10 +++++++---\n transport.c | 6 ++++--\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 127518c2a8..15ac75ab51 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -218,7 +218,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n \t\t\t _(\"Unbundling objects\"), NULL);\n \tret = !!unbundle(the_repository, &header, bundle_fd,\n-\t\t\t &extra_index_pack_args, 0) ||\n+\t\t\t &extra_index_pack_args, NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n \ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 0df66e2872..cdf9e4f9e1 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -367,6 +367,10 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \tstruct string_list_item *refname;\n \tstruct strbuf bundle_ref = STRBUF_INIT;\n \tsize_t bundle_prefix_len;\n+\tstruct unbundle_opts opts = {\n+\t\t.flags = VERIFY_BUNDLE_QUIET |\n+\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0),\n+\t};\n \n \tbundle_fd = read_bundle_header(file, &header);\n \tif (bundle_fd < 0) {\n@@ -379,8 +383,7 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \t * a reachable ref pointing to the new tips, which will reach\n \t * the prerequisite commits.\n \t */\n-\tresult = unbundle(r, &header, bundle_fd, NULL,\n-\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n+\tresult = unbundle(r, &header, bundle_fd, NULL, &opts);\n \tif (result) {\n \t\tresult = 1;\n \t\tgoto cleanup;\ndiff --git a/bundle.c b/bundle.c\nindex 4773b51eb1..485033ea3f 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -628,9 +628,13 @@ int create_bundle(struct repository *r, const char *path,\n \n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags)\n+\t struct unbundle_opts *opts)\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tenum verify_bundle_flags flags = 0;\n+\n+\tif (opts)\n+\t\tflags = opts->flags;\n \n \tif (verify_bundle(r, header, flags))\n \t\treturn -1;\ndiff --git a/bundle.h b/bundle.h\nindex 5ccc9a061a..6a09cc7bfb 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -39,6 +39,10 @@ enum verify_bundle_flags {\n int verify_bundle(struct repository *r, struct bundle_header *header,\n \t\t enum verify_bundle_flags flags);\n \n+struct unbundle_opts {\n+\tenum verify_bundle_flags flags;\n+};\n+\n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n@@ -49,12 +53,12 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n * \"extra_index_pack_args\" (if any) will be strvec_clear()'d for you.\n *\n- * Before unbundling, this method will call verify_bundle() with the\n- * given 'flags'.\n+ * Before unbundling, this method will call verify_bundle() with 'flags'\n+ * provided in 'opts'.\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags);\n+\t struct unbundle_opts *opts);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 47fda6a773..8536b14181 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -176,6 +176,9 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t\t int nr_heads UNUSED,\n \t\t\t\t struct ref **to_fetch UNUSED)\n {\n+\tstruct unbundle_opts opts = {\n+\t\t.flags = fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0,\n+\t};\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n@@ -186,8 +189,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t &extra_index_pack_args,\n-\t\t fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);\n+\t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n-- \n2.47.0\n\n"},{"id":"508204","messageId":"20241127005707.319881-3-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127005707.319881-1-jltobler@gmail.com","subject":"[PATCH v2 2/4] bundle: support fsck message configuration","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T00:57:05Z","receivedAt":"2024-11-27T01:00:04Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"If the `VERIFY_BUNDLE_FLAG` is set during `unbundle()`, the\ngit-index-pack(1) spawned is configured with the `--fsck-options` flag\nto perform fsck verification. With this flag enabled, there is not a way\nto configure fsck message severity though.\n\nExtend the `unbundle_opts` type to store fsck message severity\nconfiguration and update `unbundle()` to conditionally append it to the\n`--fsck-objects` flag if provided. This enables `unbundle()` call sites\nto support optionally setting the severity for specific fsck messages.\n\nSigned-off-by: Justin Tobler \n---\n bundle.c | 13 +++++++------\n bundle.h | 7 +++++++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 485033ea3f..4e53ddfca2 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -631,12 +631,12 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t struct unbundle_opts *opts)\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n-\tenum verify_bundle_flags flags = 0;\n+\tstruct unbundle_opts opts_fallback = { 0 };\n \n-\tif (opts)\n-\t\tflags = opts->flags;\n+\tif (!opts)\n+\t\topts = &opts_fallback;\n \n-\tif (verify_bundle(r, header, flags))\n+\tif (verify_bundle(r, header, opts->flags))\n \t\treturn -1;\n \n \tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n@@ -645,8 +645,9 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \tif (header->filter.choice)\n \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n \n-\tif (flags & VERIFY_BUNDLE_FSCK)\n-\t\tstrvec_push(&ip.args, \"--fsck-objects\");\n+\tif (opts->flags & VERIFY_BUNDLE_FSCK)\n+\t\tstrvec_pushf(&ip.args, \"--fsck-objects%s\",\n+\t\t\t opts->fsck_msg_types ? opts->fsck_msg_types : \"\");\n \n \tif (extra_index_pack_args)\n \t\tstrvec_pushv(&ip.args, extra_index_pack_args->v);\ndiff --git a/bundle.h b/bundle.h\nindex 6a09cc7bfb..df17326b09 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -41,6 +41,13 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n \n struct unbundle_opts {\n \tenum verify_bundle_flags flags;\n+\t/**\n+\t * fsck_msg_types may optionally contain fsck message severity\n+\t * configuration. If present, this configuration gets directly appended\n+\t * to a '--fsck-objects' option and therefore must be prefixed with '='.\n+\t * (E.g. \"=missingEmail=ignore,gitmodulesUrl=ignore\")\n+\t */\n+\tconst char *fsck_msg_types;\n };\n \n /**\n-- \n2.47.0\n\n"},{"id":"508205","messageId":"20241127005707.319881-4-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127005707.319881-1-jltobler@gmail.com","subject":"[PATCH v2 3/4] fetch-pack: split out fsck config parsing","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T00:57:06Z","receivedAt":"2024-11-27T01:00:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When `fetch_pack_config()` is invoked, fetch-pack configuration is\nparsed from the config. As part of this operation, fsck message severity\nconfiguration is assigned to the `fsck_msg_types` global variable. This\nis optionally used to configure the downstream git-index-pack(1) when\nthe `--strict` option is specified.\n\nThe same parsed fsck message severity configuration is also needed\noutside of fetch-pack. Instead of exposing/relying on the existing\nglobal state, split out the fsck config parsing logic into\n`fetch_pack_fsck_config()` and expose it. In a subsequent commit, this\nis used to provide fsck configuration when invoking `unbundle()`.\n\nSigned-off-by: Justin Tobler \n---\n fetch-pack.c | 24 +++++++++++++++++-------\n fetch-pack.h | 3 +++\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex fe1fb3c1b7..e7d4f6e6e2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1857,8 +1857,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n-static int fetch_pack_config_cb(const char *var, const char *value,\n-\t\t\t\tconst struct config_context *ctx, void *cb)\n+int fetch_pack_fsck_config(const char *var, const char *value,\n+\t\t\t struct strbuf *msg_types)\n {\n \tconst char *msg_id;\n \n@@ -1867,8 +1867,8 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn 1;\n-\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n-\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n+\t\t\tmsg_types->len ? ',' : '=', path);\n \t\tfree(path);\n \t\treturn 0;\n \t}\n@@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n \t\tif (is_valid_msg_type(msg_id, value))\n-\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n-\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n+\t\t\tstrbuf_addf(msg_types, \"%c%s=%s\",\n+\t\t\t\tmsg_types->len ? ',' : '=', msg_id, value);\n \t\telse\n \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, ctx, cb);\n+\treturn -1;\n+}\n+\n+static int fetch_pack_config_cb(const char *var, const char *value,\n+\t\t\t\tconst struct config_context *ctx, void *cb)\n+{\n+\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n+\tif (ret < 0)\n+\t\treturn git_default_config(var, value, ctx, cb);\n+\n+\treturn ret;\n }\n \n static void fetch_pack_config(void)\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex b5c579cdae..c667b6fbf3 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -106,4 +106,7 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);\n */\n int fetch_pack_fsck_objects(void);\n \n+int fetch_pack_fsck_config(const char *var, const char *value,\n+\t\t\t struct strbuf *msg_types);\n+\n #endif\n-- \n2.47.0\n\n"},{"id":"508206","messageId":"20241127005707.319881-5-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127005707.319881-1-jltobler@gmail.com","subject":"[PATCH v2 4/4] transport: propagate fsck configuration during bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T00:57:07Z","receivedAt":"2024-11-27T01:00:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When fetching directly from a bundle, fsck message severity\nconfiguration is not propagated to the underlying git-index-pack(1). It\nis only capable of enabling or disabling fsck checks entirely. This does\nnot align with the fsck behavior for fetches through git-fetch-pack(1).\n\nUse the fsck config parsing from fetch-pack to populate fsck message\nseverity configuration and wire it through to `unbundle()` to enable the\nsame fsck verification as done through fetch-pack.\n\nSigned-off-by: Justin Tobler \n---\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 20 ++++++++++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 7ceaa8194d..c69aa88eae 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -171,6 +171,13 @@ test_expect_success 'clone bundle with different fsckObjects configurations' '\n \n \ttest_must_fail git -c transfer.fsckObjects=true \\\n \t\tclone bundle-fsck/bad.bundle bundle-transfer-fsck 2>err &&\n+\ttest_grep \"missingEmail\" err &&\n+\n+\tgit -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=ignore \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-ignore &&\n+\n+\ttest_must_fail git -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=error \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-error 2>err &&\n \ttest_grep \"missingEmail\" err\n '\n \ndiff --git a/transport.c b/transport.c\nindex 8536b14181..0209cac5e2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -19,6 +19,7 @@\n #include \"branch.h\"\n #include \"url.h\"\n #include \"submodule.h\"\n+#include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"oid-array.h\"\n #include \"sigchain.h\"\n@@ -172,6 +173,19 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \treturn result;\n }\n \n+static int fetch_fsck_config_cb(const char *var, const char *value,\n+\t\t\t\tconst struct config_context *ctx UNUSED, void *cb)\n+{\n+\tstruct strbuf *msg_types = cb;\n+\tint ret;\n+\n+\tret = fetch_pack_fsck_config(var, value, msg_types);\n+\tif (ret < 0)\n+\t\treturn 0;\n+\n+\treturn ret;\n+}\n+\n static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t\t int nr_heads UNUSED,\n \t\t\t\t struct ref **to_fetch UNUSED)\n@@ -181,6 +195,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t};\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n+\tstruct strbuf msg_types = STRBUF_INIT;\n \tint ret;\n \n \tif (transport->progress)\n@@ -188,11 +203,16 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n+\n+\tgit_config(fetch_fsck_config_cb, &msg_types);\n+\topts.fsck_msg_types = msg_types.buf;\n+\n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n+\tstrbuf_release(&msg_types);\n \treturn ret;\n }\n \n-- \n2.47.0\n\n"},{"id":"508207","messageId":"4f43efzkdocq4lohy2nkdebhdmlxt7adaui5h3pc4cvqb4gczu@mei2h3twif3z","threadId":"62525","inReplyTo":"Z0C1PxdltxDm8pyA@ArchLinux","subject":"Re: [PATCH 4/5] fetch-pack: expose `fetch_pack_config_cb()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T01:21:12Z","receivedAt":"2024-11-27T01:23:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/23 12:45AM, shejialuo wrote:\n> On Thu, Nov 21, 2024 at 02:41:18PM -0600, Justin Tobler wrote:\n> > During fetch-pack operations, git-index-pack(1) may be spawned and\n> > perform fsck checks. The message severity of these checks is\n> > configurable and propagated via appending it to the `--fsck-objects`\n> > option.\n> > \n> > With `fetch_pack_config_cb()`, fsck configuration gets populated to a\n> > `fetch_pack_options`. Expose `fetch_pack_config_cb()`, to facilitate\n> > formatted fsck message configuration generation. In a subsequent commit,\n> > this is used to wire message configuration to `unbundle()` during bundle\n> > fetches.\n> > \n> \n> In my perspective, we may not separate [PATCH 4/5] and [PATCH 5/5].\n> Should the reason why we want to expose `fetch_pack_config_cb` is that\n> we need to propagate the fsck severity to `unbundle`? Without the\n> information of the last patch, we cannot know any detail thing. So, they\n> are highly relevant.\n\nThanks for the feedback. Due to other suggested changes, I've opted to\ncondense the third and fourth patch into one in the next version which I\nthink helps somewhat. I've also tried to update the commit messages to\nprovide additional context. Hopefully this helps :)\n\n-Justin\n"},{"id":"508209","messageId":"xmqqzfllv4ji.fsf@gitster.g","threadId":"62525","inReplyTo":"20241127005707.319881-5-jltobler@gmail.com","subject":"Re: [PATCH v2 4/4] transport: propagate fsck configuration during bundle fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-27T01:39:13Z","receivedAt":"2024-11-27T01:39:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> +static int fetch_fsck_config_cb(const char *var, const char *value,\n> +\t\t\t\tconst struct config_context *ctx UNUSED, void *cb)\n> +{\n> +\tstruct strbuf *msg_types = cb;\n> +\tint ret;\n> +\n> +\tret = fetch_pack_fsck_config(var, value, msg_types);\n> +\tif (ret < 0)\n> +\t\treturn 0;\n> +\n> +\treturn ret;\n> +}\n\nLooks good. Both callers of fetch_pack_fsck_config() must deal with\nthe unusual -1 return from it that signals \"I didn't find what I\ncare\", and the original caller does so by calling the default callback\nwhile this one just says \"everything is well\".\n\nI wasn't carefully looking for typoes and style violations, but the\nlogic flow in all these four patches looked perfect in this round.\n\nThanks for a pleasant read. Will queue.\n"},{"id":"508211","messageId":"Z0axyWrT8n8reQWl@pks.im","threadId":"62525","inReplyTo":"20241127005707.319881-3-jltobler@gmail.com","subject":"Re: [PATCH v2 2/4] bundle: support fsck message configuration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-27T05:44:47Z","receivedAt":"2024-11-27T05:45:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 26, 2024 at 06:57:05PM -0600, Justin Tobler wrote:\n> diff --git a/bundle.c b/bundle.c\n> index 485033ea3f..4e53ddfca2 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -631,12 +631,12 @@ int unbundle(struct repository *r, struct bundle_header *header,\n> \t struct unbundle_opts *opts)\n> {\n> \tstruct child_process ip = CHILD_PROCESS_INIT;\n> -\tenum verify_bundle_flags flags = 0;\n> +\tstruct unbundle_opts opts_fallback = { 0 };\n> \n> -\tif (opts)\n> -\t\tflags = opts->flags;\n> +\tif (!opts)\n> +\t\topts = &opts_fallback;\n\nTiny nit: you could've introduced the fallback in the first commit\nalready. Like this you first introduce the code pattern and then change\nit immediately in the subsequent commit.\n\nNot worth a reroll though.\n\n> diff --git a/bundle.h b/bundle.h\n> index 6a09cc7bfb..df17326b09 100644\n> --- a/bundle.h\n> +++ b/bundle.h\n> @@ -41,6 +41,13 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n> \n> struct unbundle_opts {\n> \tenum verify_bundle_flags flags;\n> +\t/**\n\nNit: s|/**/|/*|\n\nAgain, not worth a reroll from my point of view, also with the recent\ndiscussion at <877c8yti5n.fsf@iotcl.com> in mind where we basically went\n\"We don't care about them\".\n\nPatrick\n"},{"id":"508210","messageId":"Z0ax0_SDtinH6zCY@pks.im","threadId":"62525","inReplyTo":"20241127005707.319881-4-jltobler@gmail.com","subject":"Re: [PATCH v2 3/4] fetch-pack: split out fsck config parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-27T05:44:51Z","receivedAt":"2024-11-27T05:45:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 26, 2024 at 06:57:06PM -0600, Justin Tobler wrote:\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index fe1fb3c1b7..e7d4f6e6e2 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> \t\tif (is_valid_msg_type(msg_id, value))\n> -\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> -\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n> +\t\t\tstrbuf_addf(msg_types, \"%c%s=%s\",\n> +\t\t\t\tmsg_types->len ? ',' : '=', msg_id, value);\n> \t\telse\n> \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n> \t\treturn 0;\n> \t}\n> \n> -\treturn git_default_config(var, value, ctx, cb);\n> +\treturn -1;\n> +}\n> +\n> +static int fetch_pack_config_cb(const char *var, const char *value,\n> +\t\t\t\tconst struct config_context *ctx, void *cb)\n> +{\n> +\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n> +\tif (ret < 0)\n> +\t\treturn git_default_config(var, value, ctx, cb);\n> +\n> +\treturn ret;\n> }\n> \n> static void fetch_pack_config(void)\n\nOkay, this now reads a lot nicer. But I'm sceptical whether we should\nreturn -1 for the case where the value wasn't handled because we now\ncannot discern the case where the function returns an error from the\ncase where it simply didn't handle the value.\n\nIn fact, we cannot even use positive values right now as far as I can\nsee:\n\n - We return 0 on success.\n\n - We return 1 in case `git_config_pathname()` indicates an error.\n\n - We return -1 when there is no value with \"fetch.fsck.\".\n\nI'd propose to have a look at whether the positive return value from the\nsecond case is actually used anywhere. If not, we can refactor this case\nso that we always return negative on errors. And then we could further\nadapt the function to return positive in case it didn't handle the\nvalue.\n\n> diff --git a/fetch-pack.h b/fetch-pack.h\n> index b5c579cdae..c667b6fbf3 100644\n> --- a/fetch-pack.h\n> +++ b/fetch-pack.h\n> @@ -106,4 +106,7 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);\n> */\n> int fetch_pack_fsck_objects(void);\n> \n> +int fetch_pack_fsck_config(const char *var, const char *value,\n> +\t\t\t struct strbuf *msg_types);\n\nWe should also add some docs here, at the least to document the error\ncodes.\n\nPatrick\n"},{"id":"508241","messageId":"lqlchznj3gkvne3zqquz2elky36ubgh7y4n7m7wem5o3tad4bd@bvyjficofawj","threadId":"62525","inReplyTo":"Z0ax0_SDtinH6zCY@pks.im","subject":"Re: [PATCH v2 3/4] fetch-pack: split out fsck config parsing","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T17:37:12Z","receivedAt":"2024-11-27T17:39:11Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/11/27 06:44AM, Patrick Steinhardt wrote:\n> On Tue, Nov 26, 2024 at 06:57:06PM -0600, Justin Tobler wrote:\n> > diff --git a/fetch-pack.c b/fetch-pack.c\n> > index fe1fb3c1b7..e7d4f6e6e2 100644\n> > --- a/fetch-pack.c\n> > +++ b/fetch-pack.c\n> > @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> > \t\tif (!value)\n> > \t\t\treturn config_error_nonbool(var);\n> > \t\tif (is_valid_msg_type(msg_id, value))\n> > -\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> > -\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n> > +\t\t\tstrbuf_addf(msg_types, \"%c%s=%s\",\n> > +\t\t\t\tmsg_types->len ? ',' : '=', msg_id, value);\n> > \t\telse\n> > \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n> > \t\treturn 0;\n> > \t}\n> > \n> > -\treturn git_default_config(var, value, ctx, cb);\n> > +\treturn -1;\n> > +}\n> > +\n> > +static int fetch_pack_config_cb(const char *var, const char *value,\n> > +\t\t\t\tconst struct config_context *ctx, void *cb)\n> > +{\n> > +\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n> > +\tif (ret < 0)\n> > +\t\treturn git_default_config(var, value, ctx, cb);\n> > +\n> > +\treturn ret;\n> > }\n> > \n> > static void fetch_pack_config(void)\n> \n> Okay, this now reads a lot nicer. But I'm sceptical whether we should\n> return -1 for the case where the value wasn't handled because we now\n> cannot discern the case where the function returns an error from the\n> case where it simply didn't handle the value.\n> \n> In fact, we cannot even use positive values right now as far as I can\n> see:\n> \n> - We return 0 on success.\n> \n> - We return 1 in case `git_config_pathname()` indicates an error.\n> \n> - We return -1 when there is no value with \"fetch.fsck.\".\n> \n> I'd propose to have a look at whether the positive return value from the\n> second case is actually used anywhere. If not, we can refactor this case\n> so that we always return negative on errors. And then we could further\n> adapt the function to return positive in case it didn't handle the\n> value.\n\nThe `fetch_pack_fsck_config()` function is only used through callback\nfunctions for `git_config()`. The `config_fn_t` callback is expected to\nreturn 0 for success, or -1 if the variable could not be parsed\nproperly. Taking a look at `configset_iter()`, in practice any return\nvalue >= 0 is treated as success.\n\nThis means that because the `git_config_pathname()` errors in\n`fetch_pack_fsck_config()` return 1, an error is printed, but the\noperation is able to proceed.\n\n # Skip list is missing path value.\n $ git -c fetch.fsck.skipList fetch\n error: missing value for 'fetch.fsck.skiplist'\n \n\nThis skip list flag behavior is consistent across other fsck operations.\nChanging it here to return -1 would make config parsing more strict and\nresult in a missing skip list path causing the whole operation to die.\n\nSince returning 1 is treated as a success anyway, we could change it to\nreturn 0. This would result in functionally the same behavior and free\nup `fetch_pack_fsck_config()` to return 1 to indicate that there is no\nvalue with \"fetch.fsck.\". This would allow us to properly differentiate\nbetween the \"didn't handle the value\" and error cases.\n\nI'll send another version making this change and add some comments to\ndocument the behavior.\n\n-Justin\n"},{"id":"508248","messageId":"20241127233312.27710-1-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127005707.319881-1-jltobler@gmail.com","subject":"[PATCH v3 0/4] propagate fsck message severity for bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T23:33:08Z","receivedAt":"2024-11-27T23:35:48Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"Greetings,\n\nWith 63d903ff52 (unbundle: extend object verification for fetches,\n2024-06-19), fsck checks are now performed on fetched bundles depending\non `transfer.fsckObjects` and `fetch.fsckObjects` configuration. This\nworks, but provides no means to override the default fsck message\nseverity as is done for other git-fetch(1) operations. This series aims\nto propagate fsck message severity configuration to the underlying\ngit-index-pack(1) process executed on the bundle in a similar manner as\ndone with git-fetch-pack(1).\n\n - Patches 1 and 2 adapt the bundle subsystem to support additional\n options when performing `unbundle()`.\n\n - Patch 3 adapts the fetch-pack subsystem to expose a means to\n generate the message configuration arguments.\n\n - Patch 4 wires the newly generated fsck configuration options to the\n bundle options used when fetching from a bundle.\n\nChanges in V3:\n\n - The `fetch_pack_fsck_config()` has been updated to now return 1 when\n the provided config variable is undhandled instead of returning -1.\n This allows call sites to properly differentiate between errors and\n unhandled config variables. To make this change, the handling of\n `git_config_path()` errors was updated to return 0 instead of 1.\n Both of these return values considered by `git_config()` to be a\n success so there is no functional change. This allows returning 1 to\n now indicate that the config variable was not process only.\n\n - Added comment to document expected `fetch_pack_fsck_config()`\n behavior and return values.\n\n - Small comment style change.\n\nThanks\n-Justin\n\nJustin Tobler (4):\n bundle: add bundle verification options type\n bundle: support fsck message configuration\n fetch-pack: split out fsck config parsing\n transport: propagate fsck configuration during bundle fetch\n\n builtin/bundle.c | 2 +-\n bundle-uri.c | 7 +++++--\n bundle.c | 13 +++++++++----\n bundle.h | 17 ++++++++++++++---\n fetch-pack.c | 26 ++++++++++++++++++--------\n fetch-pack.h | 11 +++++++++++\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 26 ++++++++++++++++++++++++--\n 8 files changed, 89 insertions(+), 20 deletions(-)\n\nRange-diff against v2:\n1: da47f0aa0f = 1: da47f0aa0f bundle: add bundle verification options type\n2: 19e91c9f99 ! 2: 5dbd0fa6b7 bundle: support fsck message configuration\n @@ bundle.h: int verify_bundle(struct repository *r, struct bundle_header *header,\n \n struct unbundle_opts {\n \tenum verify_bundle_flags flags;\n -+\t/**\n ++\t/*\n +\t * fsck_msg_types may optionally contain fsck message severity\n +\t * configuration. If present, this configuration gets directly appended\n +\t * to a '--fsck-objects' option and therefore must be prefixed with '='.\n3: 527874e73d ! 3: b8db9af9e7 fetch-pack: split out fsck config parsing\n @@ Commit message\n `fetch_pack_fsck_config()` and expose it. In a subsequent commit, this\n is used to provide fsck configuration when invoking `unbundle()`.\n \n + For `fetch_pack_fsck_config()` to discern between errors and unhandled\n + config variables, the return code when `git_config_path()` errors is\n + changed to a different value also indicating success. This frees up the\n + previous return code to now indicate the provided config variable\n + was unhandled. The behavior remains functionally the same.\n +\n Signed-off-by: Justin Tobler \n \n ## fetch-pack.c ##\n @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tconst char *msg_id;\n \n @@ fetch-pack.c: static int fetch_pack_config_cb(const char *var, const char *value,\n + \t\tchar *path ;\n \n \t\tif (git_config_pathname(&path, var, value))\n - \t\t\treturn 1;\n +-\t\t\treturn 1;\n -\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n -\t\t\tfsck_msg_types.len ? ',' : '=', path);\n ++\t\t\treturn 0;\n +\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n +\t\t\tmsg_types->len ? ',' : '=', path);\n \t\tfree(path);\n @@ fetch-pack.c: static int fetch_pack_config_cb(const char *var, const char *value\n \t}\n \n -\treturn git_default_config(var, value, ctx, cb);\n -+\treturn -1;\n ++\treturn 1;\n +}\n +\n +static int fetch_pack_config_cb(const char *var, const char *value,\n +\t\t\t\tconst struct config_context *ctx, void *cb)\n +{\n +\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n -+\tif (ret < 0)\n ++\tif (ret > 0)\n +\t\treturn git_default_config(var, value, ctx, cb);\n +\n +\treturn ret;\n @@ fetch-pack.h: int report_unmatched_refs(struct ref **sought, int nr_sought);\n */\n int fetch_pack_fsck_objects(void);\n \n ++/*\n ++ * Check if the provided config variable pertains to fetch fsck and if so append\n ++ * the configuration to the provided strbuf.\n ++ *\n ++ * When a fetch fsck config option is successfully processed the function\n ++ * returns 0. If the provided config option is unrelated to fetch fsck, 1 is\n ++ * returned. Errors return -1.\n ++ */\n +int fetch_pack_fsck_config(const char *var, const char *value,\n +\t\t\t struct strbuf *msg_types);\n +\n4: b1a3f73561 ! 4: cc8ae0a1c4 transport: propagate fsck configuration during bundle fetch\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport\n +\tint ret;\n +\n +\tret = fetch_pack_fsck_config(var, value, msg_types);\n -+\tif (ret < 0)\n ++\tif (ret > 0)\n +\t\treturn 0;\n +\n +\treturn ret;\n\nbase-commit: 4083a6f05206077a50af7658bedc17a94c54607d\n-- \n2.47.0\n\n"},{"id":"508249","messageId":"20241127233312.27710-2-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127233312.27710-1-jltobler@gmail.com","subject":"[PATCH v3 1/4] bundle: add bundle verification options type","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T23:33:09Z","receivedAt":"2024-11-27T23:35:52Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When `unbundle()` is invoked, fsck verification may be configured by\npassing the `VERIFY_BUNDLE_FSCK` flag. This mechanism allows fsck checks\non the bundle to be enabled or disabled entirely. To facilitate more\nfine-grained fsck configuration, additional context must be provided to\n`unbundle()`.\n\nIntroduce the `unbundle_opts` type, which wraps the existing\n`verify_bundle_flags`, to facilitate future extension of `unbundle()`\nconfiguration. Also update `unbundle()` and its call sites to accept\nthis new options type instead of the flags directly. The end behavior is\nfunctionally the same, but allows for the set of configurable options to\nbe extended. This is leveraged in a subsequent commit to enable fsck\nmessage severity configuration.\n\nSigned-off-by: Justin Tobler \n---\n builtin/bundle.c | 2 +-\n bundle-uri.c | 7 +++++--\n bundle.c | 6 +++++-\n bundle.h | 10 +++++++---\n transport.c | 6 ++++--\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 127518c2a8..15ac75ab51 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -218,7 +218,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n \t\t\t _(\"Unbundling objects\"), NULL);\n \tret = !!unbundle(the_repository, &header, bundle_fd,\n-\t\t\t &extra_index_pack_args, 0) ||\n+\t\t\t &extra_index_pack_args, NULL) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n \tbundle_header_release(&header);\n \ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 0df66e2872..cdf9e4f9e1 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -367,6 +367,10 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \tstruct string_list_item *refname;\n \tstruct strbuf bundle_ref = STRBUF_INIT;\n \tsize_t bundle_prefix_len;\n+\tstruct unbundle_opts opts = {\n+\t\t.flags = VERIFY_BUNDLE_QUIET |\n+\t\t\t (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0),\n+\t};\n \n \tbundle_fd = read_bundle_header(file, &header);\n \tif (bundle_fd < 0) {\n@@ -379,8 +383,7 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \t * a reachable ref pointing to the new tips, which will reach\n \t * the prerequisite commits.\n \t */\n-\tresult = unbundle(r, &header, bundle_fd, NULL,\n-\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n+\tresult = unbundle(r, &header, bundle_fd, NULL, &opts);\n \tif (result) {\n \t\tresult = 1;\n \t\tgoto cleanup;\ndiff --git a/bundle.c b/bundle.c\nindex 4773b51eb1..485033ea3f 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -628,9 +628,13 @@ int create_bundle(struct repository *r, const char *path,\n \n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags)\n+\t struct unbundle_opts *opts)\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n+\tenum verify_bundle_flags flags = 0;\n+\n+\tif (opts)\n+\t\tflags = opts->flags;\n \n \tif (verify_bundle(r, header, flags))\n \t\treturn -1;\ndiff --git a/bundle.h b/bundle.h\nindex 5ccc9a061a..6a09cc7bfb 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -39,6 +39,10 @@ enum verify_bundle_flags {\n int verify_bundle(struct repository *r, struct bundle_header *header,\n \t\t enum verify_bundle_flags flags);\n \n+struct unbundle_opts {\n+\tenum verify_bundle_flags flags;\n+};\n+\n /**\n * Unbundle after reading the header with read_bundle_header().\n *\n@@ -49,12 +53,12 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n * (e.g. \"-v\" for verbose/progress), NULL otherwise. The provided\n * \"extra_index_pack_args\" (if any) will be strvec_clear()'d for you.\n *\n- * Before unbundling, this method will call verify_bundle() with the\n- * given 'flags'.\n+ * Before unbundling, this method will call verify_bundle() with 'flags'\n+ * provided in 'opts'.\n */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, struct strvec *extra_index_pack_args,\n-\t enum verify_bundle_flags flags);\n+\t struct unbundle_opts *opts);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n \ndiff --git a/transport.c b/transport.c\nindex 47fda6a773..8536b14181 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -176,6 +176,9 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t\t int nr_heads UNUSED,\n \t\t\t\t struct ref **to_fetch UNUSED)\n {\n+\tstruct unbundle_opts opts = {\n+\t\t.flags = fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0,\n+\t};\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n \tint ret;\n@@ -186,8 +189,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n \tret = unbundle(the_repository, &data->header, data->fd,\n-\t\t &extra_index_pack_args,\n-\t\t fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);\n+\t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n-- \n2.47.0\n\n"},{"id":"508250","messageId":"20241127233312.27710-3-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127233312.27710-1-jltobler@gmail.com","subject":"[PATCH v3 2/4] bundle: support fsck message configuration","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T23:33:10Z","receivedAt":"2024-11-27T23:35:55Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"If the `VERIFY_BUNDLE_FLAG` is set during `unbundle()`, the\ngit-index-pack(1) spawned is configured with the `--fsck-options` flag\nto perform fsck verification. With this flag enabled, there is not a way\nto configure fsck message severity though.\n\nExtend the `unbundle_opts` type to store fsck message severity\nconfiguration and update `unbundle()` to conditionally append it to the\n`--fsck-objects` flag if provided. This enables `unbundle()` call sites\nto support optionally setting the severity for specific fsck messages.\n\nSigned-off-by: Justin Tobler \n---\n bundle.c | 13 +++++++------\n bundle.h | 7 +++++++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 485033ea3f..4e53ddfca2 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -631,12 +631,12 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t struct unbundle_opts *opts)\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n-\tenum verify_bundle_flags flags = 0;\n+\tstruct unbundle_opts opts_fallback = { 0 };\n \n-\tif (opts)\n-\t\tflags = opts->flags;\n+\tif (!opts)\n+\t\topts = &opts_fallback;\n \n-\tif (verify_bundle(r, header, flags))\n+\tif (verify_bundle(r, header, opts->flags))\n \t\treturn -1;\n \n \tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n@@ -645,8 +645,9 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \tif (header->filter.choice)\n \t\tstrvec_push(&ip.args, \"--promisor=from-bundle\");\n \n-\tif (flags & VERIFY_BUNDLE_FSCK)\n-\t\tstrvec_push(&ip.args, \"--fsck-objects\");\n+\tif (opts->flags & VERIFY_BUNDLE_FSCK)\n+\t\tstrvec_pushf(&ip.args, \"--fsck-objects%s\",\n+\t\t\t opts->fsck_msg_types ? opts->fsck_msg_types : \"\");\n \n \tif (extra_index_pack_args)\n \t\tstrvec_pushv(&ip.args, extra_index_pack_args->v);\ndiff --git a/bundle.h b/bundle.h\nindex 6a09cc7bfb..a80aa8ad9b 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -41,6 +41,13 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n \n struct unbundle_opts {\n \tenum verify_bundle_flags flags;\n+\t/*\n+\t * fsck_msg_types may optionally contain fsck message severity\n+\t * configuration. If present, this configuration gets directly appended\n+\t * to a '--fsck-objects' option and therefore must be prefixed with '='.\n+\t * (E.g. \"=missingEmail=ignore,gitmodulesUrl=ignore\")\n+\t */\n+\tconst char *fsck_msg_types;\n };\n \n /**\n-- \n2.47.0\n\n"},{"id":"508251","messageId":"20241127233312.27710-4-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127233312.27710-1-jltobler@gmail.com","subject":"[PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T23:33:11Z","receivedAt":"2024-11-27T23:35:58Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When `fetch_pack_config()` is invoked, fetch-pack configuration is\nparsed from the config. As part of this operation, fsck message severity\nconfiguration is assigned to the `fsck_msg_types` global variable. This\nis optionally used to configure the downstream git-index-pack(1) when\nthe `--strict` option is specified.\n\nThe same parsed fsck message severity configuration is also needed\noutside of fetch-pack. Instead of exposing/relying on the existing\nglobal state, split out the fsck config parsing logic into\n`fetch_pack_fsck_config()` and expose it. In a subsequent commit, this\nis used to provide fsck configuration when invoking `unbundle()`.\n\nFor `fetch_pack_fsck_config()` to discern between errors and unhandled\nconfig variables, the return code when `git_config_path()` errors is\nchanged to a different value also indicating success. This frees up the\nprevious return code to now indicate the provided config variable\nwas unhandled. The behavior remains functionally the same.\n\nSigned-off-by: Justin Tobler \n---\n fetch-pack.c | 26 ++++++++++++++++++--------\n fetch-pack.h | 11 +++++++++++\n 2 files changed, 29 insertions(+), 8 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex fe1fb3c1b7..c095f3a84b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1857,8 +1857,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n-static int fetch_pack_config_cb(const char *var, const char *value,\n-\t\t\t\tconst struct config_context *ctx, void *cb)\n+int fetch_pack_fsck_config(const char *var, const char *value,\n+\t\t\t struct strbuf *msg_types)\n {\n \tconst char *msg_id;\n \n@@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \t\tchar *path ;\n \n \t\tif (git_config_pathname(&path, var, value))\n-\t\t\treturn 1;\n-\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n-\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\t\treturn 0;\n+\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n+\t\t\tmsg_types->len ? ',' : '=', path);\n \t\tfree(path);\n \t\treturn 0;\n \t}\n@@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n \t\tif (is_valid_msg_type(msg_id, value))\n-\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n-\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n+\t\t\tstrbuf_addf(msg_types, \"%c%s=%s\",\n+\t\t\t\tmsg_types->len ? ',' : '=', msg_id, value);\n \t\telse\n \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, ctx, cb);\n+\treturn 1;\n+}\n+\n+static int fetch_pack_config_cb(const char *var, const char *value,\n+\t\t\t\tconst struct config_context *ctx, void *cb)\n+{\n+\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n+\tif (ret > 0)\n+\t\treturn git_default_config(var, value, ctx, cb);\n+\n+\treturn ret;\n }\n \n static void fetch_pack_config(void)\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex b5c579cdae..9d3470366f 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -106,4 +106,15 @@ int report_unmatched_refs(struct ref **sought, int nr_sought);\n */\n int fetch_pack_fsck_objects(void);\n \n+/*\n+ * Check if the provided config variable pertains to fetch fsck and if so append\n+ * the configuration to the provided strbuf.\n+ *\n+ * When a fetch fsck config option is successfully processed the function\n+ * returns 0. If the provided config option is unrelated to fetch fsck, 1 is\n+ * returned. Errors return -1.\n+ */\n+int fetch_pack_fsck_config(const char *var, const char *value,\n+\t\t\t struct strbuf *msg_types);\n+\n #endif\n-- \n2.47.0\n\n"},{"id":"508252","messageId":"20241127233312.27710-5-jltobler@gmail.com","threadId":"62525","inReplyTo":"20241127233312.27710-1-jltobler@gmail.com","subject":"[PATCH v3 4/4] transport: propagate fsck configuration during bundle fetch","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-11-27T23:33:12Z","receivedAt":"2024-11-27T23:35:59Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"When fetching directly from a bundle, fsck message severity\nconfiguration is not propagated to the underlying git-index-pack(1). It\nis only capable of enabling or disabling fsck checks entirely. This does\nnot align with the fsck behavior for fetches through git-fetch-pack(1).\n\nUse the fsck config parsing from fetch-pack to populate fsck message\nseverity configuration and wire it through to `unbundle()` to enable the\nsame fsck verification as done through fetch-pack.\n\nSigned-off-by: Justin Tobler \n---\n t/t5607-clone-bundle.sh | 7 +++++++\n transport.c | 20 ++++++++++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 7ceaa8194d..c69aa88eae 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -171,6 +171,13 @@ test_expect_success 'clone bundle with different fsckObjects configurations' '\n \n \ttest_must_fail git -c transfer.fsckObjects=true \\\n \t\tclone bundle-fsck/bad.bundle bundle-transfer-fsck 2>err &&\n+\ttest_grep \"missingEmail\" err &&\n+\n+\tgit -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=ignore \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-ignore &&\n+\n+\ttest_must_fail git -c fetch.fsckObjects=true -c fetch.fsck.missingEmail=error \\\n+\t\tclone bundle-fsck/bad.bundle bundle-fsck-error 2>err &&\n \ttest_grep \"missingEmail\" err\n '\n \ndiff --git a/transport.c b/transport.c\nindex 8536b14181..6966df51a8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -19,6 +19,7 @@\n #include \"branch.h\"\n #include \"url.h\"\n #include \"submodule.h\"\n+#include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"oid-array.h\"\n #include \"sigchain.h\"\n@@ -172,6 +173,19 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \treturn result;\n }\n \n+static int fetch_fsck_config_cb(const char *var, const char *value,\n+\t\t\t\tconst struct config_context *ctx UNUSED, void *cb)\n+{\n+\tstruct strbuf *msg_types = cb;\n+\tint ret;\n+\n+\tret = fetch_pack_fsck_config(var, value, msg_types);\n+\tif (ret > 0)\n+\t\treturn 0;\n+\n+\treturn ret;\n+}\n+\n static int fetch_refs_from_bundle(struct transport *transport,\n \t\t\t\t int nr_heads UNUSED,\n \t\t\t\t struct ref **to_fetch UNUSED)\n@@ -181,6 +195,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t};\n \tstruct bundle_transport_data *data = transport->data;\n \tstruct strvec extra_index_pack_args = STRVEC_INIT;\n+\tstruct strbuf msg_types = STRBUF_INIT;\n \tint ret;\n \n \tif (transport->progress)\n@@ -188,11 +203,16 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \n \tif (!data->get_refs_from_bundle_called)\n \t\tget_refs_from_bundle_inner(transport);\n+\n+\tgit_config(fetch_fsck_config_cb, &msg_types);\n+\topts.fsck_msg_types = msg_types.buf;\n+\n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t &extra_index_pack_args, &opts);\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n+\tstrbuf_release(&msg_types);\n \treturn ret;\n }\n \n-- \n2.47.0\n\n"},{"id":"508260","messageId":"xmqqed2wox8n.fsf@gitster.g","threadId":"62525","inReplyTo":"20241127233312.27710-4-jltobler@gmail.com","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-28T03:25:44Z","receivedAt":"2024-11-28T03:25:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n> For `fetch_pack_fsck_config()` to discern between errors and unhandled\n> config variables, the return code when `git_config_path()` errors is\n> changed to a different value also indicating success. This frees up the\n> previous return code to now indicate the provided config variable\n> was unhandled. The behavior remains functionally the same.\n\n> @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> \t\tchar *path ;\n> \n> \t\tif (git_config_pathname(&path, var, value))\n> -\t\t\treturn 1;\n> +\t\t\treturn 0;\n\nSo, after getting complaint about a misconfiguration, the caller\nused to be able to, if they wanted to, tell two cases apart: a\nmisconfigured variable was ignored here, and we happily parsed the\nconfiguration. Now they no longer can tell them apart, because we\nreturn 0 for both cases.\n\n> -\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n> -\t\t\tfsck_msg_types.len ? ',' : '=', path);\n> +\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n> +\t\t\tmsg_types->len ? ',' : '=', path);\n> \t\tfree(path);\n> \t\treturn 0;\n> \t}\n> @@ -1877,14 +1877,24 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> \t\tif (!value)\n> \t\t\treturn config_error_nonbool(var);\n> \t\tif (is_valid_msg_type(msg_id, value))\n> -\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> -\t\t\t\tfsck_msg_types.len ? ',' : '=', msg_id, value);\n> +\t\t\tstrbuf_addf(msg_types, \"%c%s=%s\",\n> +\t\t\t\tmsg_types->len ? ',' : '=', msg_id, value);\n> \t\telse\n> \t\t\twarning(\"Skipping unknown msg id '%s'\", msg_id);\n> \t\treturn 0;\n> \t}\n> \n> -\treturn git_default_config(var, value, ctx, cb);\n> +\treturn 1;\n> +}\n\nAnd a 1 is returned to signal a \"we didn't see what we care about\".\n\n> +static int fetch_pack_config_cb(const char *var, const char *value,\n> +\t\t\t\tconst struct config_context *ctx, void *cb)\n> +{\n> +\tint ret = fetch_pack_fsck_config(var, value, &fsck_msg_types);\n> +\tif (ret > 0)\n> +\t\treturn git_default_config(var, value, ctx, cb);\n> +\n> +\treturn ret;\n> }\n\nAnd we catch that 1 and ask git_default_config() to use what we\ndidn't use.\n\nOK. If I were doing this patch, I would probably have chosen not to\nchange the value used to signal \"a misconfiguration but that is not\ntoo serious so we'll ignore after warning\", picked another and new\nvalue, like 2, to signal \"the key is not something we care about\",\nwhich would mean fetch_pack_config_cb() would call \"default\" only\nwhen it sees 2. But with the current callers, and with the callers\nafter this series, it is not necessary. If we need to accomodate\nother new callers later, we can make such an update then.\n\nThis step, together with other patches, are nicely done.\n\nWill queue. Thanks.\n"},{"id":"508521","messageId":"Z07QqowPVEgJ3sIw@pks.im","threadId":"62525","inReplyTo":"xmqqed2wox8n.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-03T09:34:44Z","receivedAt":"2024-12-03T09:35:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 28, 2024 at 12:25:44PM +0900, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> > For `fetch_pack_fsck_config()` to discern between errors and unhandled\n> > config variables, the return code when `git_config_path()` errors is\n> > changed to a different value also indicating success. This frees up the\n> > previous return code to now indicate the provided config variable\n> > was unhandled. The behavior remains functionally the same.\n> \n> > @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> > \t\tchar *path ;\n> > \n> > \t\tif (git_config_pathname(&path, var, value))\n> > -\t\t\treturn 1;\n> > +\t\t\treturn 0;\n> \n> So, after getting complaint about a misconfiguration, the caller\n> used to be able to, if they wanted to, tell two cases apart: a\n> misconfigured variable was ignored here, and we happily parsed the\n> configuration. Now they no longer can tell them apart, because we\n> return 0 for both cases.\n\nI wonder why we want to ignore these errors though. Grepping through the\ncodebase surfaces that all other callsites of `git_config_pathname()`\nreturn its return value directly, which means that we'll die in case the\npath name cannot be parsed. Shouldn't we do the same here, or is there a\ngood reason why we need to ignore it other than \"We used to do it like\nthat\"? In other words, I would have the below diff.\n\nOther than that this patch series looks good to me.\n\nPatrick\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex fe1fb3c1b7c..c60627bba4e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1863,10 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n \tconst char *msg_id;\n \n \tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n-\t\tchar *path ;\n+\t\tchar *path;\n+\t\tint err;\n+\n+\t\terr = git_config_pathname(&path, var, value);\n+\t\tif (err < 0)\n+\t\t\treturn -1;\n \n-\t\tif (git_config_pathname(&path, var, value))\n-\t\t\treturn 1;\n \t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n \t\t\tfsck_msg_types.len ? ',' : '=', path);\n \t\tfree(path);\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 138e6778a47..73d6b3874de 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -240,6 +240,15 @@ test_expect_success 'push with receive.fsck.skipList' '\n \tgit push --porcelain dst bogus\n '\n \n+test_expect_success 'fetch with implitcit fetch.fsck.skipList value fails' '\n+\ttest_when_finished \"rm -rf source repo\" &&\n+\tgit init source &&\n+\ttest_commit -C source initial &&\n+\tgit init repo &&\n+\ttest_must_fail git -C repo -c fetch.fsck.skipList fetch \"file://$(pwd)/source\" 2>err &&\n+\ttest_grep \"unable to parse ${SQ}fetch.fsck.skiplist${SQ}\" err\n+'\n+\n test_expect_success 'fetch with fetch.fsck.skipList' '\n \trefspec=refs/heads/bogus:refs/heads/bogus &&\n \tgit push . $commit:refs/heads/bogus &&\n"},{"id":"508526","messageId":"i7tvwk462uwe3syarrnmjttgu4p7urtymq4n72kj7ziaptu7iy@vh4td5fd24ko","threadId":"62525","inReplyTo":"Z07QqowPVEgJ3sIw@pks.im","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-12-03T14:23:11Z","receivedAt":"2024-12-03T14:25:16Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/03 10:34AM, Patrick Steinhardt wrote:\n> On Thu, Nov 28, 2024 at 12:25:44PM +0900, Junio C Hamano wrote:\n> > Justin Tobler writes:\n> > \n> > > For `fetch_pack_fsck_config()` to discern between errors and unhandled\n> > > config variables, the return code when `git_config_path()` errors is\n> > > changed to a different value also indicating success. This frees up the\n> > > previous return code to now indicate the provided config variable\n> > > was unhandled. The behavior remains functionally the same.\n> > \n> > > @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> > > \t\tchar *path ;\n> > > \n> > > \t\tif (git_config_pathname(&path, var, value))\n> > > -\t\t\treturn 1;\n> > > +\t\t\treturn 0;\n> > \n> > So, after getting complaint about a misconfiguration, the caller\n> > used to be able to, if they wanted to, tell two cases apart: a\n> > misconfigured variable was ignored here, and we happily parsed the\n> > configuration. Now they no longer can tell them apart, because we\n> > return 0 for both cases.\n> \n> I wonder why we want to ignore these errors though. Grepping through the\n> codebase surfaces that all other callsites of `git_config_pathname()`\n> return its return value directly, which means that we'll die in case the\n> path name cannot be parsed. Shouldn't we do the same here, or is there a\n> good reason why we need to ignore it other than \"We used to do it like\n> that\"? In other words, I would have the below diff.\n\nIn both \"fsck.c:git_fsck_config()\" and\n\"receive-pack:receive_pack_config()\", the `git_config_pathname()`\ncallsites are set to return 1 on error. The main reason for ignoring the\nerror was to keep it consistent with the other fsck operations usage.\nOutside of that, I also wasn't quite sure why specifically\n\"fsck.skipList\" parsing is not as strict.\n\nI do think being consistent would be nice, but I don't feel super\nstrongly either way. I'm open to changing it in another version if that\nis best. :)\n\n-Justin\n"},{"id":"508527","messageId":"Z08VoOHGZ1wecUx6@pks.im","threadId":"62525","inReplyTo":"i7tvwk462uwe3syarrnmjttgu4p7urtymq4n72kj7ziaptu7iy@vh4td5fd24ko","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-03T14:28:48Z","receivedAt":"2024-12-03T14:29:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 03, 2024 at 08:23:11AM -0600, Justin Tobler wrote:\n> On 24/12/03 10:34AM, Patrick Steinhardt wrote:\n> > On Thu, Nov 28, 2024 at 12:25:44PM +0900, Junio C Hamano wrote:\n> > > Justin Tobler writes:\n> > > \n> > > > For `fetch_pack_fsck_config()` to discern between errors and unhandled\n> > > > config variables, the return code when `git_config_path()` errors is\n> > > > changed to a different value also indicating success. This frees up the\n> > > > previous return code to now indicate the provided config variable\n> > > > was unhandled. The behavior remains functionally the same.\n> > > \n> > > > @@ -1866,9 +1866,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,\n> > > > \t\tchar *path ;\n> > > > \n> > > > \t\tif (git_config_pathname(&path, var, value))\n> > > > -\t\t\treturn 1;\n> > > > +\t\t\treturn 0;\n> > > \n> > > So, after getting complaint about a misconfiguration, the caller\n> > > used to be able to, if they wanted to, tell two cases apart: a\n> > > misconfigured variable was ignored here, and we happily parsed the\n> > > configuration. Now they no longer can tell them apart, because we\n> > > return 0 for both cases.\n> > \n> > I wonder why we want to ignore these errors though. Grepping through the\n> > codebase surfaces that all other callsites of `git_config_pathname()`\n> > return its return value directly, which means that we'll die in case the\n> > path name cannot be parsed. Shouldn't we do the same here, or is there a\n> > good reason why we need to ignore it other than \"We used to do it like\n> > that\"? In other words, I would have the below diff.\n> \n> In both \"fsck.c:git_fsck_config()\" and\n> \"receive-pack:receive_pack_config()\", the `git_config_pathname()`\n> callsites are set to return 1 on error. The main reason for ignoring the\n> error was to keep it consistent with the other fsck operations usage.\n> Outside of that, I also wasn't quite sure why specifically\n> \"fsck.skipList\" parsing is not as strict.\n\nOkay, so your refactorings are simply keeping the status quo and are\nconsistent with what we do elsewhere where we parse the same config key.\n\n> I do think being consistent would be nice, but I don't feel super\n> strongly either way. I'm open to changing it in another version if that\n> is best. :)\n\nI think the current version of this patch series is fine as-is then. It\nmight be a good idea to adapt this in a follow-up series, unless there\nis a good reason not to.\n\nPatrick\n"},{"id":"508580","messageId":"xmqqy10w9x0m.fsf@gitster.g","threadId":"62525","inReplyTo":"Z08VoOHGZ1wecUx6@pks.im","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T23:17:45Z","receivedAt":"2024-12-03T23:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I think the current version of this patch series is fine as-is then. It\n> might be a good idea to adapt this in a follow-up series, unless there\n> is a good reason not to.\n\nSounds good. We may want to tighten the rules a bit to reject\nobvious misconfigurations, but that is outside the scope of this\ntopic.\n"},{"id":"508595","messageId":"xmqq34j45fzr.fsf@gitster.g","threadId":"62525","inReplyTo":"xmqqy10w9x0m.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] fetch-pack: split out fsck config parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T02:39:04Z","receivedAt":"2024-12-04T02:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\n>\n>> I think the current version of this patch series is fine as-is then. It\n>> might be a good idea to adapt this in a follow-up series, unless there\n>> is a good reason not to.\n>\n> Sounds good. We may want to tighten the rules a bit to reject\n> obvious misconfigurations, but that is outside the scope of this\n> topic.\n\nThis reminds me of a slightly related tangent.\n\nThere are pathname-valued ones that we added deliberately as an\noptional configuration variable, IIRC, and fsck.skiplist may be one\nof them. In other words, they mean \"In a repository that I want the\nconfiguration to affect, I'll have the configuration file at this\npath, but it is merely optional. If it is missing, pretend as if\nthe configuration variable does not even exist\".\n\nIn retrospect, it was a mistake to define that this variable is\nrequired and triggers an error when misconfigured, and that variable\nis optional and triggers only a warning when misconfigured. That is\none more thing for the user to remember and does not scale.\n\nWe probably should have done something like\n\n - the value given to all pathname valued configuration variables\n and command name options MUST correctly name a path that the\n command can read, and a misconfigured configuration variable or\n an invalid command line option should trigger an error when the\n command needs to access it.\n\n - the value (not the variable) can say \"I am optional--if the path\n does not appear on this system, or if it is unreadable, pretend\n as if this configuration variable or the command line option were\n not given\".\n\nThe \"when the command needs to\" part is important. Ideally, \"git\nlog\" should not fail when core.hookspath is misconfigured, because\nit does not trigger any hook, but it currently dies while parsing\nthe configuration files in git_default_config():\n\n $ git -c core.hookspath log\n error: missing value for 'core.hookspath'\n fatal: unable to parse 'core.hookspath' from command-line config.\n\nwhich we may want to fix. Unsurprisingly there are other variables\nthat do behave correctly, like\n\n $ git -c core.pager --no-pager log -20 >/dev/null\n $ git -c core.pager log -20 >/dev/null\n error: missing value for 'core.pager'\n fatal: unable to parse command-line config\n\nwhere a misconfigured core.pager does not cause any trouble when the\npager is not in use.\n\nAny of the above are not within the scope of this topic, obviously.\n\ncf. <20241014204427.1712182-1-gitster@pobox.com>\n"}]}