| Message ID | 20210106114727.8405-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Æ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. > > 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. > > 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 &&
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. >> >> 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. 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 &&
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 &&