Re: [PATCH v6 23/23] mktag: add a --[no-]strict option
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 6, 2021, 22:21 UTC
- Message-ID
- <xmqqmtxlpuyg.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20210106114727.8405-1-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 30 quoted lines
> Now that mktag has been migrated to use the fsck machinery to check > its input, it makes sense to teach it to run in the equivalent of "git > fsck"'s default mode. > > For cases where mktag is used to (re)create a tag object using data > from an existing and malformed tag object, the validation may > optionally have to be loosened. Teach the command to take the > "--[no-]strict" option to do so. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > > On Wed, Jan 06 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> Now that mktag has been migrated to use the fsck machinery to check >>> its input, it makes sense to teach it to run in the equivalent of "git >>> fsck"'s default mode, instead of hardcoding "git fsck --strict". Let's >>> do that and support the "--no-strict" option. >>> >>> Since this is a new option we don't need to cater to parse-option.c's >>> default of automatically supporting --strict. So let's use >>> PARSE_OPT_NONEG, using a new trivial helper macro. >> >> I do not understand the last paragraph. > > I forgot to update the commit message when changing this from > v4->v5. Here's a v6 of just this patch (to save on list traffic) with > an updated commit message. Thanks!
Okay. I actually was expecting to hear the reason why the approach taken in v4 to use PARSE_OPT_NONEG is a good one, but I take the change of approach in v5 as the sign that you now agree with me that a plain vanilla bool that happens to default to true is easier to manage.
Will replace. Thanks.
Show 153 quoted lines
>
> Documentation/git-mktag.txt | 8 ++++++++
> builtin/mktag.c | 9 +++++++++
> t/t3800-mktag.sh | 33 +++++++++++++++++++++++----------
> 3 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-mktag.txt b/Documentation/git-mktag.txt
> index 79813ff8df0..17a2603a600 100644
> --- a/Documentation/git-mktag.txt
> +++ b/Documentation/git-mktag.txt
> @@ -11,6 +11,14 @@ SYNOPSIS
> [verse]
> 'git mktag'
>
> +OPTIONS
> +-------
> +
> +--strict::
> + By default mktag turns on the equivalent of
> + linkgit:git-fsck[1] `--strict` mode. Use `--no-strict` to
> + disable it.
> +
> DESCRIPTION
> -----------
>
> diff --git a/builtin/mktag.c b/builtin/mktag.c
> index 9b04b61c2bb..41a399a69e4 100644
> --- a/builtin/mktag.c
> +++ b/builtin/mktag.c
> @@ -10,6 +10,7 @@ static char const * const builtin_mktag_usage[] = {
> N_("git mktag"),
> NULL
> };
> +static int option_strict = 1;
>
> static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
>
> @@ -25,6 +26,12 @@ static int mktag_fsck_error_func(struct fsck_options *o,
> {
> switch (msg_type) {
> case FSCK_WARN:
> + if (!option_strict) {
> + fprintf_ln(stderr, _("warning: tag input does not pass fsck: %s"), message);
> + return 0;
> +
> + }
> + /* fallthrough */
> case FSCK_ERROR:
> /*
> * We treat both warnings and errors as errors, things
> @@ -67,6 +74,8 @@ static int verify_object_in_tag(struct object_id *tagged_oid, int *tagged_type)
> int cmd_mktag(int argc, const char **argv, const char *prefix)
> {
> static struct option builtin_mktag_options[] = {
> + OPT_BOOL(0, "strict", &option_strict,
> + N_("enable more strict checking")),
> OPT_END(),
> };
> struct strbuf buf = STRBUF_INIT;
> diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
> index 0ddd0f01cc7..4d76ccbbc19 100755
> --- a/t/t3800-mktag.sh
> +++ b/t/t3800-mktag.sh
> @@ -12,12 +12,17 @@ test_description='git mktag: tag object verify test'
> # given in the expect.pat file.
>
> check_verify_failure () {
> - expect="$2"
> - test_expect_success "$1" '
> + test_expect_success "$1" "
> test_must_fail env GIT_TEST_GETTEXT_POISON=false \
> git mktag <tag.sig 2>message &&
> - grep "$expect" message
> - '
> + grep '$2' message &&
> + if test '$3' != '--no-strict'
> + then
> + test_must_fail env GIT_TEST_GETTEXT_POISON=false \
> + git mktag --no-strict <tag.sig 2>message.no-strict &&
> + grep '$2' message.no-strict
> + fi
> + "
> }
>
> test_expect_mktag_success() {
> @@ -65,7 +70,7 @@ too short for a tag
> EOF
>
> check_verify_failure 'Tag object length check' \
> - '^error:.* missingObject:'
> + '^error:.* missingObject:' 'strict'
>
> ############################################################
> # 2. object line label check
> @@ -240,7 +245,7 @@ tagger . <> 0 +0000
> EOF
>
> check_verify_failure 'verify tag-name check' \
> - '^error:.* badTagName:'
> + '^error:.* badTagName:' '--no-strict'
>
> ############################################################
> # 11. tagger line label check #1
> @@ -254,7 +259,7 @@ This is filler
> EOF
>
> check_verify_failure '"tagger" line label check #1' \
> - '^error:.* missingTaggerEntry:'
> + '^error:.* missingTaggerEntry:' '--no-strict'
>
> ############################################################
> # 12. tagger line label check #2
> @@ -269,7 +274,7 @@ This is filler
> EOF
>
> check_verify_failure '"tagger" line label check #2' \
> - '^error:.* missingTaggerEntry:'
> + '^error:.* missingTaggerEntry:' '--no-strict'
>
> ############################################################
> # 13. allow missing tag author name like fsck
> @@ -298,7 +303,7 @@ tagger T A Gger <
> EOF
>
> check_verify_failure 'disallow malformed tagger' \
> - '^error:.* badEmail:'
> + '^error:.* badEmail:' '--no-strict'
>
> ############################################################
> # 15. allow empty tag email
> @@ -422,13 +427,21 @@ this line should not be here
> EOF
>
> check_verify_failure 'detect invalid header entry' \
> - '^error:.* extraHeaderEntry:'
> + '^error:.* extraHeaderEntry:' '--no-strict'
>
> test_expect_success 'invalid header entry config & fsck' '
> test_must_fail git mktag <tag.sig &&
> + git mktag --no-strict <tag.sig &&
> +
> test_must_fail git -c fsck.extraHeaderEntry=error mktag <tag.sig &&
> + test_must_fail git -c fsck.extraHeaderEntry=error mktag --no-strict <tag.sig &&
> +
> test_must_fail git -c fsck.extraHeaderEntry=warn mktag <tag.sig &&
> + git -c fsck.extraHeaderEntry=warn mktag --no-strict <tag.sig &&
> +
> git -c fsck.extraHeaderEntry=ignore mktag <tag.sig &&
> + git -c fsck.extraHeaderEntry=ignore mktag --no-strict <tag.sig &&
> +
> git fsck &&
> env GIT_TEST_GETTEXT_POISON=false \
> git -c fsck.extraHeaderEntry=warn fsck 2>err &&