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

[Junio C Hamano <gitster@pobox.com>]

"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.