On Wed, Jan 06 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
>>
>> 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@xxxxxxxxx>
>> ---
>>
>> On Wed, Jan 06 2021, Junio C Hamano wrote:
>>
>>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
There was no good reason. I couldn't find whether &no_var with "no-OPT"
options & PARSE_OPT_NONEG was the preferred pattern or &var with a
default of 1 with "OPT". There's a few examples of the former in the
codebase, but I'm happy to go with the latter.
>>
>> 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 &&
