{"thread":{"id":"62760","subject":"[PATCH] fsck: reject misconfigured fsck.skipList","startedAt":"2025-01-07T16:33:09Z","lastAt":"2025-01-09T08:00:21Z","messageCount":3,"participants":["Justin Tobler","Junio C Hamano","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"510093","messageId":"20250107162914.3756968-2-jltobler@gmail.com","threadId":"62760","inReplyTo":null,"subject":"[PATCH] fsck: reject misconfigured fsck.skipList","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-01-07T16:29:15Z","receivedAt":"2025-01-07T16:33:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"In Git, fsck operations can ignore known broken objects via the\n`fsck.skipList` configuration. This option expects a path to a file with\nthe list of object names. When the configuration is specified without a\npath, an error message is printed, but the command continues as if the\nconfiguration was not set. Configuring `fsck.skipList` without a value\nis a misconfiguration so config parsing should be more strict and reject\nit.\n\nUpdate `git_fsck_config()` to no longer ignore misconfiguration of\n`fsck.skipList`. The same behavior is also present for\n`fetch.fsck.skipList` and `receive.fsck.skipList` so the configuration\nparsers for these are updated to ensure the related operations remain\nconsistent.\n\nSigned-off-by: Justin Tobler <jltobler@gmail.com>\n---\nThis patch is a follow up to previous discussion in [1] regarding how a\nmisconfigured `fsck.skipList` without a path is handled.\n\nThanks,\n-Justin\n\n[1]: <xmqqy10w9x0m.fsf@gitster.g>\n---\n builtin/receive-pack.c          |  2 +-\n fetch-pack.c                    |  2 +-\n fsck.c                          |  2 +-\n t/t5504-fetch-receive-strict.sh | 10 ++++++++++\n 4 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c2e9103f11..0158faf537 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -174,7 +174,7 @@ static int receive_pack_config(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\tfree(path);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 3a227721ed..055e8c3643 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1867,7 +1867,7 @@ int fetch_pack_fsck_config(const char *var, const char *value,\n \t\tchar *path ;\n \n \t\tif (git_config_pathname(&path, var, value))\n-\t\t\treturn 0;\n+\t\t\treturn -1;\n \t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n \t\t\tmsg_types->len ? ',' : '=', path);\n \t\tfree(path);\ndiff --git a/fsck.c b/fsck.c\nindex 87ce999a49..9fc4c25ffd 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1353,7 +1353,7 @@ int git_fsck_config(const char *var, const char *value,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (git_config_pathname(&path, var, value))\n-\t\t\treturn 1;\n+\t\t\treturn -1;\n \t\tstrbuf_addf(&sb, \"skiplist=%s\", path);\n \t\tfree(path);\n \t\tfsck_set_msg_types(options, sb.buf);\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 8212a70be8..e273ab29c7 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -167,6 +167,8 @@ test_expect_success 'fsck with unsorted skipList' '\n \n test_expect_success 'fsck with invalid or bogus skipList input' '\n \tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n+\ttest_must_fail git -c fsck.skipList -c fsck.missingEmail=ignore fsck 2>err &&\n+\ttest_grep \"unable to parse '\\'fsck.skiplist\\'' from command-line config\" err &&\n \ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n \ttest_grep \"could not open.*: does-not-exist\" err &&\n \ttest_must_fail git -c fsck.skipList=.git/config -c fsck.missingEmail=ignore fsck 2>err &&\n@@ -213,6 +215,11 @@ test_expect_success 'fsck with exhaustive accepted skipList input (various types\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'receive-pack with missing receive.fsck.skipList path' '\n+\ttest_must_fail git -c receive.fsck.skipList receive-pack dst 2>err &&\n+\ttest_grep \"unable to parse '\\'receive.fsck.skiplist\\'' from command-line config\" err\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tgit push . $commit:refs/heads/bogus &&\n \trm -rf dst &&\n@@ -255,6 +262,9 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n \n \t# Invalid and/or bogus skipList input\n+\ttest_must_fail git --git-dir=dst/.git -c fetch.fsck.skipList fetch \\\n+\t\t\"file://$(pwd)\" $refspec 2>err &&\n+\ttest_grep \"unable to parse '\\'fetch.fsck.skiplist\\'' from command-line config\" err &&\n \tgit --git-dir=dst/.git config fetch.fsck.skipList /dev/null &&\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n \tgit --git-dir=dst/.git config fetch.fsck.skipList does-not-exist &&\n\nbase-commit: b74ff38af58464688b211140b90ec90598d340c6\n-- \n2.47.1\n\n"},{"id":"510100","messageId":"xmqqttaatu8h.fsf@gitster.g","threadId":"62760","inReplyTo":"20250107162914.3756968-2-jltobler@gmail.com","subject":"Re: [PATCH] fsck: reject misconfigured fsck.skipList","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-07T17:21:50Z","receivedAt":"2025-01-07T17:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> In Git, fsck operations can ignore known broken objects via the\n> `fsck.skipList` configuration. This option expects a path to a file with\n> the list of object names. When the configuration is specified without a\n> path, an error message is printed, but the command continues as if the\n> configuration was not set. Configuring `fsck.skipList` without a value\n> is a misconfiguration so config parsing should be more strict and reject\n> it.\n>\n> Update `git_fsck_config()` to no longer ignore misconfiguration of\n> `fsck.skipList`. The same behavior is also present for\n> `fetch.fsck.skipList` and `receive.fsck.skipList` so the configuration\n> parsers for these are updated to ensure the related operations remain\n> consistent.\n\nIf the value is missing, i.e.,\n\n\t[fsck]\n\t\tskipList\n\nit is a very clear misconfiguration.  \"We expect a path, but you\ngave me a valueless true\".  Once a specified value gets to\noidset_parse_file(), we would die when a specified path cannot be\nopened, so it is not like we want to deliberately tolerate\nmisconfiguration (we also die if the value is given as \"~t/sl\" and\nuser \"t\" does not exist on the system).\n\nMakes sense.\n"},{"id":"510222","messageId":"87jzb4ju2j.fsf@iotcl.com","threadId":"62760","inReplyTo":"xmqqttaatu8h.fsf@gitster.g","subject":"Re: [PATCH] fsck: reject misconfigured fsck.skipList","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2025-01-09T08:00:04Z","receivedAt":"2025-01-09T08:00:21Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>\n> If the value is missing, i.e.,\n>\n> \t[fsck]\n> \t\tskipList\n>\n> it is a very clear misconfiguration.  \"We expect a path, but you\n> gave me a valueless true\".  Once a specified value gets to\n> oidset_parse_file(), we would die when a specified path cannot be\n> opened, so it is not like we want to deliberately tolerate\n> misconfiguration (we also die if the value is given as \"~t/sl\" and\n> user \"t\" does not exist on the system).\n>\n> Makes sense.\n\nYeah, I did some testing as well and reviewed the code. Also makes sense\nto me.\n\n--\nToon\n"}]}