| Message ID | 20250107162914.3756968-2-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fsck: reject misconfigured fsck.skipList | expand |
Justin Tobler <jltobler@gmail.com> writes: > In Git, fsck operations can ignore known broken objects via the > `fsck.skipList` configuration. This option expects a path to a file with > the list of object names. When the configuration is specified without a > path, an error message is printed, but the command continues as if the > configuration was not set. Configuring `fsck.skipList` without a value > is a misconfiguration so config parsing should be more strict and reject > it. > > Update `git_fsck_config()` to no longer ignore misconfiguration of > `fsck.skipList`. The same behavior is also present for > `fetch.fsck.skipList` and `receive.fsck.skipList` so the configuration > parsers for these are updated to ensure the related operations remain > consistent. If the value is missing, i.e., [fsck] skipList it is a very clear misconfiguration. "We expect a path, but you gave me a valueless true". Once a specified value gets to oidset_parse_file(), we would die when a specified path cannot be opened, so it is not like we want to deliberately tolerate misconfiguration (we also die if the value is given as "~t/sl" and user "t" does not exist on the system). Makes sense.
Junio C Hamano <gitster@pobox.com> writes: > > If the value is missing, i.e., > > [fsck] > skipList > > it is a very clear misconfiguration. "We expect a path, but you > gave me a valueless true". Once a specified value gets to > oidset_parse_file(), we would die when a specified path cannot be > opened, so it is not like we want to deliberately tolerate > misconfiguration (we also die if the value is given as "~t/sl" and > user "t" does not exist on the system). > > Makes sense. Yeah, I did some testing as well and reviewed the code. Also makes sense to me. -- Toon
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c2e9103f11..0158faf537 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -174,7 +174,7 @@ static int receive_pack_config(const char *var, const char *value, char *path; if (git_config_pathname(&path, var, value)) - return 1; + return -1; strbuf_addf(&fsck_msg_types, "%cskiplist=%s", fsck_msg_types.len ? ',' : '=', path); free(path); diff --git a/fetch-pack.c b/fetch-pack.c index 3a227721ed..055e8c3643 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1867,7 +1867,7 @@ int fetch_pack_fsck_config(const char *var, const char *value, char *path ; if (git_config_pathname(&path, var, value)) - return 0; + return -1; strbuf_addf(msg_types, "%cskiplist=%s", msg_types->len ? ',' : '=', path); free(path); diff --git a/fsck.c b/fsck.c index 87ce999a49..9fc4c25ffd 100644 --- a/fsck.c +++ b/fsck.c @@ -1353,7 +1353,7 @@ int git_fsck_config(const char *var, const char *value, struct strbuf sb = STRBUF_INIT; if (git_config_pathname(&path, var, value)) - return 1; + return -1; strbuf_addf(&sb, "skiplist=%s", path); free(path); fsck_set_msg_types(options, sb.buf); diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh index 8212a70be8..e273ab29c7 100755 --- a/t/t5504-fetch-receive-strict.sh +++ b/t/t5504-fetch-receive-strict.sh @@ -167,6 +167,8 @@ test_expect_success 'fsck with unsorted skipList' ' test_expect_success 'fsck with invalid or bogus skipList input' ' git -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck && + test_must_fail git -c fsck.skipList -c fsck.missingEmail=ignore fsck 2>err && + test_grep "unable to parse '\'fsck.skiplist\'' from command-line config" err && test_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err && test_grep "could not open.*: does-not-exist" err && test_must_fail git -c fsck.skipList=.git/config -c fsck.missingEmail=ignore fsck 2>err && @@ -213,6 +215,11 @@ test_expect_success 'fsck with exhaustive accepted skipList input (various types test_must_be_empty err ' +test_expect_success 'receive-pack with missing receive.fsck.skipList path' ' + test_must_fail git -c receive.fsck.skipList receive-pack dst 2>err && + test_grep "unable to parse '\'receive.fsck.skiplist\'' from command-line config" err +' + test_expect_success 'push with receive.fsck.skipList' ' git push . $commit:refs/heads/bogus && rm -rf dst && @@ -255,6 +262,9 @@ test_expect_success 'fetch with fetch.fsck.skipList' ' test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec && # Invalid and/or bogus skipList input + test_must_fail git --git-dir=dst/.git -c fetch.fsck.skipList fetch \ + "file://$(pwd)" $refspec 2>err && + test_grep "unable to parse '\'fetch.fsck.skiplist\'' from command-line config" err && git --git-dir=dst/.git config fetch.fsck.skipList /dev/null && test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec && git --git-dir=dst/.git config fetch.fsck.skipList does-not-exist &&
In Git, fsck operations can ignore known broken objects via the `fsck.skipList` configuration. This option expects a path to a file with the list of object names. When the configuration is specified without a path, an error message is printed, but the command continues as if the configuration was not set. Configuring `fsck.skipList` without a value is a misconfiguration so config parsing should be more strict and reject it. Update `git_fsck_config()` to no longer ignore misconfiguration of `fsck.skipList`. The same behavior is also present for `fetch.fsck.skipList` and `receive.fsck.skipList` so the configuration parsers for these are updated to ensure the related operations remain consistent. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- This patch is a follow up to previous discussion in [1] regarding how a misconfigured `fsck.skipList` without a path is handled. Thanks, -Justin [1]: <xmqqy10w9x0m.fsf@gitster.g> --- builtin/receive-pack.c | 2 +- fetch-pack.c | 2 +- fsck.c | 2 +- t/t5504-fetch-receive-strict.sh | 10 ++++++++++ 4 files changed, 13 insertions(+), 3 deletions(-) base-commit: b74ff38af58464688b211140b90ec90598d340c6