fsck: reject misconfigured fsck.skipList

Commit Message

Justin Tobler Jan. 7, 2025, 4:29 p.m. UTC

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 <[e-mail]>
---
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

Comments

Junio C Hamano Jan. 7, 2025, 5:21 p.m. UTC | #1

Justin Tobler <[e-mail]> 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.

Patch

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 &&