diff mbox series

[2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs

Message ID 074e0c7ab923777c66516ced18b4fd1dadf7677f.1706215884.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series index-pack: fsck honor checks | expand

Commit Message

John Cai Jan. 25, 2024, 8:51 p.m. UTC 
From: John Cai <johncai86@gmail.com>

git-index-pack has a --strict mode that can take an optional argument to
provide a list of fsck issues to change their severity. --fsck-objects
does not have such a utility, which would be useful if one would like to
be more lenient or strict on data integrity in a repository.

Like --strict, Allow --fsck-objects to also take a list of fsck msgs to
change the severity.

This commit also removes the "For internal use only" note for
--fsck-objects, and documents the option. This won't often be used by
the normal end user, but it turns out it is useful for Git forges like
GitLab.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 Documentation/git-index-pack.txt | 10 ++++++++--
 builtin/index-pack.c             |  5 +++--
 t/t5300-pack-object.sh           | 29 ++++++++++++++++++++++++-----
 3 files changed, 35 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Jan. 25, 2024, 11:13 p.m. UTC | #1 
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> git-index-pack has a --strict mode that can take an optional argument to

"mode" -> "option", probably.

> provide a list of fsck issues to change their severity. --fsck-objects
> does not have such a utility, which would be useful if one would like to
> be more lenient or strict on data integrity in a repository.
>
> Like --strict, Allow --fsck-objects to also take a list of fsck msgs to
> change the severity.

"Allow" -> "allow".

> This commit also removes the "For internal use only" note for
> --fsck-objects, and documents the option. This won't often be used by
> the normal end user, but it turns out it is useful for Git forges like
> GitLab.

"This commit also removes", "documents" -> "Remove", "document".

> ---fsck-objects::
> -	For internal use only.
> +--fsck-objects[=<msg-ids>]::
> +	Instructs index-pack to check for broken objects instead of broken
> +	links. If `<msg-ids>` is passed, it should be  a comma-separated list of

Very much pleased to see an additional description that is written
to clarify the difference between this option and the other --strict
option.  The original was totally unclear on this point, and it is
very much appreciated.

The other option notices and dies upon seeing either a broken object
or a dangling link.  This one only diagnoses broken objects and does
not care if the objects are connected.  However, saying "instead of"
here tempts readers to mistakenly think that the other one only
checks links and this one only checks contents, which is not what we
want to say.  Perhaps "to check for broken objects, but unlike
`--strict`, do not choke on broken links" or something?

Same comment on <msg-ids> as the previous step.

> +	`<msg-id>=<severity>` where `<msg-id>` and `<severity>` are used to
> +	change the severity of `fsck` errors, eg: `--strict="missingEmail=ignore,badTagName=ignore"`.

Same comment for "eg:" as before.

> @@ -1785,8 +1785,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
>  			} else if (!strcmp(arg, "--check-self-contained-and-connected")) {
>  				strict = 1;
>  				check_self_contained_and_connected = 1;
> -			} else if (!strcmp(arg, "--fsck-objects")) {
> +			} else if (skip_to_optional_arg(arg, "--fsck-objects", &arg)) {
>  				do_fsck_object = 1;
> +				fsck_set_msg_types(&fsck_options, arg);
>  			} else if (!strcmp(arg, "--verify")) {
>  				verify = 1;
>  			} else if (!strcmp(arg, "--verify-stat")) {

The implementation of this part looks quite obvious, once you see
how "--strict[=<msgid>=<level>]" is implemented.

Looking good.

Thanks.
diff mbox series

Patch

diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt
index 14f806d07d1..37709b13c88 100644
--- a/Documentation/git-index-pack.txt
+++ b/Documentation/git-index-pack.txt
@@ -96,8 +96,14 @@  default and "Indexing objects" when `--stdin` is specified.
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 
---fsck-objects::
-	For internal use only.
+--fsck-objects[=<msg-ids>]::
+	Instructs index-pack to check for broken objects instead of broken
+	links. If `<msg-ids>` is passed, it should be  a comma-separated list of
+	`<msg-id>=<severity>` where `<msg-id>` and `<severity>` are used to
+	change the severity of `fsck` errors, eg: `--strict="missingEmail=ignore,badTagName=ignore"`.
+	See the entry for the `fsck.<msg-id>` configuration options in
+	`linkgit:git-fsck[1] for more information on the possible values of
+	`<msg-id>` and `<severity>`.
 +
 Die if the pack contains broken objects. If the pack contains a tree
 pointing to a .gitmodules blob that does not exist, prints the hash of
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1e53ca23775..519162f5b91 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -24,7 +24,7 @@ 
 #include "setup.h"
 
 static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-ids>]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-ids>]] [--fsck-objects[=<msg-ids>]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
 
 struct object_entry {
 	struct pack_idx_entry idx;
@@ -1785,8 +1785,9 @@  int cmd_index_pack(int argc, const char **argv, const char *prefix)
 			} else if (!strcmp(arg, "--check-self-contained-and-connected")) {
 				strict = 1;
 				check_self_contained_and_connected = 1;
-			} else if (!strcmp(arg, "--fsck-objects")) {
+			} else if (skip_to_optional_arg(arg, "--fsck-objects", &arg)) {
 				do_fsck_object = 1;
+				fsck_set_msg_types(&fsck_options, arg);
 			} else if (!strcmp(arg, "--verify")) {
 				verify = 1;
 			} else if (!strcmp(arg, "--verify-stat")) {
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 9563372ae27..916cf939beb 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -441,8 +441,7 @@  test_expect_success 'index-pack with --strict' '
 	)
 '
 
-test_expect_success 'index-pack with --strict downgrading fsck msgs' '
-	test_when_finished rm -rf strict &&
+test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
 	git init strict &&
 	(
 		cd strict &&
@@ -457,12 +456,32 @@  test_expect_success 'index-pack with --strict downgrading fsck msgs' '
 
 		EOF
 		git hash-object --literally -t commit -w --stdin <commit >commit_list &&
-		PACK=$(git pack-objects test <commit_list) &&
-		test_must_fail git index-pack --strict "test-$PACK.pack" &&
-		git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
+		git pack-objects test <commit_list >pack-name
 	)
 '
 
+test_with_bad_commit () {
+	must_fail_arg="$1" &&
+	must_pass_arg="$2" &&
+	(
+		cd strict &&
+		test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
+		git index-pack "$must_pass_arg" "test-$(cat pack-name).pack"
+	)
+}
+
+test_expect_success 'index-pack with --strict downgrading fsck msgs' '
+	test_with_bad_commit --strict --strict="missingEmail=ignore"
+'
+
+test_expect_success 'index-pack with --fsck-objects downgrading fsck msgs' '
+	test_with_bad_commit --fsck-objects --fsck-objects="missingEmail=ignore"
+'
+
+test_expect_success 'cleanup for --strict and --fsck-objects downgrading fsck msgs' '
+	rm -rf strict
+'
+
 test_expect_success 'honor pack.packSizeLimit' '
 	git config pack.packSizeLimit 3m &&
 	packname_10=$(git pack-objects test-10 <obj-list) &&