{"thread":{"id":"47876","subject":"[GSoC][PATCH] tag: Make \"git tag --contains <id>\" less chatty if <id> is invalid","startedAt":"2018-02-19T21:22:19Z","lastAt":"2018-02-20T23:29:51Z","messageCount":3,"participants":["Paul-Sebastian Ungureanu","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"339639","messageId":"20180219212130.4217-1-ungureanupaulsebastian@gmail.com","threadId":"47876","inReplyTo":null,"subject":"[GSoC][PATCH] tag: Make \"git tag --contains <id>\" less chatty if <id> is invalid","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-02-19T21:21:30Z","receivedAt":"2018-02-19T21:22:19Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"git tag --contains <id> prints the whole help text if <id> is\ninvalid. It should only show the error message instead.\n\nThis bug was a side effect of looking up the commit in option\nparser callback. When a error occurs in the option parser, the\nfull usage is shown. To fix this bug, the part related to\nlooking up the commit was moved outside of the option parser\nto the ref-filter module.\n\nBasically, the option parser only parses strings that represent\ncommits and the ref-filter performs the commit look-up. If an\nerror occurs during the option parsing, then it must be an invalid\nargument and the user should be informed of usage, but if a error\noccurs during ref-filtering, then it is a problem with the\nargument.\n\nSigned-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>\n---\n builtin/tag.c           | 11 +++++++++--\n parse-options.h         | 11 +++++++++++\n ref-filter.c            | 19 +++++++++++++++++++\n ref-filter.h            |  3 +++\n t/t7013-tag-contains.sh | 37 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 79 insertions(+), 2 deletions(-)\n create mode 100755 t/t7013-tag-contains.sh\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a7e6a5b0f..3fce14713 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -396,6 +396,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \t\tOPT_GROUP(N_(\"Tag listing options\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"show tag list in columns\")),\n+\n+\t\tOPT_CONTAINS_STRS(&filter.with_commit_strs, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_NO_CONTAINS_STRS(&filter.no_commit_strs, N_(\"print only tags that don't contain the commit\")),\n+\t\tOPT_WITH_STRS(&filter.with_commit_strs, N_(\"print only tags that contain the commit\")),\n+\t\tOPT_WITHOUT_STRS(&filter.no_commit_strs, N_(\"print only tags that don't contain the commit\")),\n+\n \t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n \t\tOPT_NO_CONTAINS(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_WITH(&filter.with_commit, N_(\"print only tags that contain the commit\")),\n@@ -437,6 +443,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcmdmode = 'l';\n \t\telse if (filter.with_commit || filter.no_commit ||\n \t\t\t filter.points_at.nr || filter.merge_commit ||\n+\t\t\t filter.with_commit_strs.nr || filter.no_commit_strs.nr ||\n \t\t\t filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n@@ -473,9 +480,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (filter.lines != -1)\n \t\tdie(_(\"-n option is only allowed in list mode\"));\n-\tif (filter.with_commit)\n+\tif (filter.with_commit || filter.with_commit_strs.nr)\n \t\tdie(_(\"--contains option is only allowed in list mode\"));\n-\tif (filter.no_commit)\n+\tif (filter.no_commit || filter.no_commit_strs.nr)\n \t\tdie(_(\"--no-contains option is only allowed in list mode\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\ndiff --git a/parse-options.h b/parse-options.h\nindex af711227a..3aa8ddd46 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -258,9 +258,20 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);\n \t  PARSE_OPT_LASTARG_DEFAULT | flag, \\\n \t  parse_opt_commits, (intptr_t) \"HEAD\" \\\n \t}\n+#define _OPT_CONTAINS_OR_WITH_STRS(name, variable, help, flag) \\\n+\t{ OPTION_CALLBACK, 0, name, (variable), N_(\"commit\"), (help), \\\n+\t  PARSE_OPT_LASTARG_DEFAULT | flag, \\\n+\t  parse_opt_string_list, (intptr_t) \"HEAD\" \\\n+\t}\n+\n #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"contains\", v, h, PARSE_OPT_NONEG)\n #define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"no-contains\", v, h, PARSE_OPT_NONEG)\n #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH(\"with\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n #define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH(\"without\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n \n+#define OPT_CONTAINS_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"contains\", v, h, PARSE_OPT_NONEG)\n+#define OPT_NO_CONTAINS_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"no-contains\", v, h, PARSE_OPT_NONEG)\n+#define OPT_WITH_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"with\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n+#define OPT_WITHOUT_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"without\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n+\n #endif\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f9e25aea7..e7bdaa27f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2000,6 +2000,21 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \tfree(to_clear);\n }\n \n+int add_str_to_commit_list(struct string_list_item *item, void *commit_list)\n+{\n+\tstruct object_id oid;\n+\tstruct commit *commit;\n+\n+\tif (get_oid(item->string, &oid))\n+\t\tdie(_(\"malformed object name %s\"), item->string);\n+\tcommit = lookup_commit_reference(&oid);\n+\tif (!commit)\n+\t\tdie(_(\"no such commit %s\"), item->string);\n+\tcommit_list_insert(commit, commit_list);\n+\n+\treturn 0;\n+}\n+\n /*\n  * API for filtering a set of refs. Based on the type of refs the user\n  * has requested, we iterate through those refs and apply filters\n@@ -2012,6 +2027,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tint ret = 0;\n \tunsigned int broken = 0;\n \n+\t/* Convert string representation and add to commit list. */\n+\tfor_each_string_list(&filter->with_commit_strs, add_str_to_commit_list, &filter->with_commit);\n+\tfor_each_string_list(&filter->no_commit_strs, add_str_to_commit_list, &filter->no_commit);\n+\n \tref_cbdata.array = array;\n \tref_cbdata.filter = filter;\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0d98342b3..62f37760f 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -55,6 +55,9 @@ struct ref_filter {\n \tstruct commit_list *with_commit;\n \tstruct commit_list *no_commit;\n \n+\tstruct string_list with_commit_strs;\n+\tstruct string_list no_commit_strs;\n+\n \tenum {\n \t\tREF_FILTER_MERGED_NONE = 0,\n \t\tREF_FILTER_MERGED_INCLUDE,\ndiff --git a/t/t7013-tag-contains.sh b/t/t7013-tag-contains.sh\nnew file mode 100755\nindex 000000000..65119dada\n--- /dev/null\n+++ b/t/t7013-tag-contains.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='Test if git tag --contains prints only the error\n+when used correctly but with an inexistent tag as argument'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\tgit init . &&\n+\techo \"this is a test\" >file &&\n+\tgit add -A &&\n+\tgit commit -am \"tag test\" &&\n+\tgit tag \"v1.0\" &&\n+\tgit tag \"v1.1\"\n+'\n+\n+test_expect_success 'tag --contains <existent_tag>' '\n+\t! (git tag --contains \"v1.0\" 2>&1 | grep -o \"usage\")\n+'\n+\n+test_expect_success 'tag --contains <existent_tag>' '\n+\t! (git tag --contains \"v1.1\" 2>&1 | grep -o \"usage\")\n+'\n+\n+test_expect_success 'tag --contains <inexistent_tag>' '\n+\t! (git tag --contains \"notag\" 2>&1 | grep -o \"usage\")\n+'\n+\n+test_expect_success 'tag --contains <inexistent_tag>' '\n+\t! (git tag --contains \"v1.2\" 2>&1 | grep -o \"usage\")\n+'\n+\n+test_expect_success 'tag usage error' '\n+\tgit tag --sort 2>&1 | grep -o \"usage\"\n+'\n+\n+test_done\n-- \n2.16.1.194.gb2e45c695.dirty\n\n"},{"id":"339757","messageId":"CAGZ79kbXXN_nxFZUorsbFHcg4-8H7ZnAOnaViUrnP4r4u6ThhQ@mail.gmail.com","threadId":"47876","inReplyTo":"20180219212130.4217-1-ungureanupaulsebastian@gmail.com","subject":"Re: [GSoC][PATCH] tag: Make \"git tag --contains <id>\" less chatty if <id> is invalid","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-02-20T22:35:49Z","receivedAt":"2018-02-20T22:35:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Welcome to the Git mailing list!\n\nOn Mon, Feb 19, 2018 at 1:21 PM, Paul-Sebastian Ungureanu\n<ungureanupaulsebastian@gmail.com> wrote:\n> git tag --contains <id> prints the whole help text if <id> is\n> invalid. It should only show the error message instead.\n\nMakes sense.\n\n>\n> This bug was a side effect of looking up the commit in option\n> parser callback. When a error occurs in the option parser, the\n> full usage is shown. To fix this bug, the part related to\n> looking up the commit was moved outside of the option parser\n> to the ref-filter module.\n>\n> Basically, the option parser only parses strings that represent\n> commits and the ref-filter performs the commit look-up. If an\n> error occurs during the option parsing, then it must be an invalid\n> argument and the user should be informed of usage, but if a error\n> occurs during ref-filtering, then it is a problem with the\n> argument.\n>\n> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>\n> ---\n\n> diff --git a/parse-options.h b/parse-options.h\n> index af711227a..3aa8ddd46 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n\nparse-options.h is a very generic place in Gits source code,\nso this would also fix 'git branch --contains=<id>' at the same time?\nWould it make sense to say so in the commit message or have a\ntest for that?\n\n> @@ -258,9 +258,20 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);\n>           PARSE_OPT_LASTARG_DEFAULT | flag, \\\n>           parse_opt_commits, (intptr_t) \"HEAD\" \\\n>         }\n> +#define _OPT_CONTAINS_OR_WITH_STRS(name, variable, help, flag) \\\n> +       { OPTION_CALLBACK, 0, name, (variable), N_(\"commit\"), (help), \\\n> +         PARSE_OPT_LASTARG_DEFAULT | flag, \\\n> +         parse_opt_string_list, (intptr_t) \"HEAD\" \\\n> +       }\n\nThis is the same as _OPT_CONTAINS_OR_WITH\nexcept parse_opt_commits is substituted by parse_opt_string_list.\n\nDo we need both? (As far as I understand this addresses a whole class\nof errors that could be eliminated, we do not want to fix git-tag only,\nwe'd also want to fix git-branch as well as git-for-each-ref ?)\nSo instead convert all callers to the new behavior, or would that break\nexisting conventions? (Then there is no need to have 2 competing\ndefines that are nearly identical) Instead of double-defining, would it\nbe possible to turn it into a flag? OPT_DONT_HELP_ON_BAD_OBJECTID\nor something?\n\n> +\n>  #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"contains\", v, h, PARSE_OPT_NONEG)\n>  #define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH(\"no-contains\", v, h, PARSE_OPT_NONEG)\n>  #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH(\"with\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n>  #define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH(\"without\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n>\n> +#define OPT_CONTAINS_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"contains\", v, h, PARSE_OPT_NONEG)\n> +#define OPT_NO_CONTAINS_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"no-contains\", v, h, PARSE_OPT_NONEG)\n> +#define OPT_WITH_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"with\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n> +#define OPT_WITHOUT_STRS(v, h) _OPT_CONTAINS_OR_WITH_STRS(\"without\", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)\n> +\n\n\n> diff --git a/t/t7013-tag-contains.sh b/t/t7013-tag-contains.sh\n> new file mode 100755\n> index 000000000..65119dada\n> --- /dev/null\n> +++ b/t/t7013-tag-contains.sh\n\nThanks for adding the tests into a new file instead of putting it somewhere\nwhere it is already convenient. (We have too many of those \"just add it there\nas it is easiest to fit in\")\n\n> +\n> +test_expect_success 'tag --contains <existent_tag>' '\n> +       ! (git tag --contains \"v1.0\" 2>&1 | grep -o \"usage\")\n\nIn the test suite we assume everything but Git flawless, but\nGit *may* be faulty. What if Git crashes (segfault) ?\nThen this test is still producing a valid \"ok\" as grep\ndoesn't find \"usage\". This pattern of piping output of\nGit into other commands is around the test suite unfortunately,\nbut we'd want to not add this pattern in new code. So maybe:\n\n    git tag --contains v1.0 2 >error &&\n    grep \"usage\" err\n\nAnother thing on this: we'd want to check the return code of\ngit tag in this case.\n\nIn case of an error in parse-opt we error out with 129\njust as git would without this patch:\n\n $ git tag --contains=a6d7eb2c7a6a402a938824bcf1c5f331dd1a06bc\nerror: no such commit a6d7eb2c7a6a402a938824bcf1c5f331dd1a06bc\nusage: git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n<tagname> [<head>]\n   or: git tag -d <tagname>...\n   or: git tag -l [-n[<num>]] [--contains <commit>] [--no-contains\n<commit>] [--points-at <object>]\n[....]\n $ echo $?\n129\n\nBut after applying this patch, we also do not want to have 129\nas the return code as that indicates that parsing the options was\nunsuccessful. Instead we want to see that the --contains operation was\nunsucessful, maybe?\n\nSo I'd expect the return code to be 0 (if we don't care) or 1\n(if we do care), in the case of 1, we shall write:\n\n  test_must_fail git tag --contains ... &&\n  grep ....\n\n(A long way of hinting at the test_must_fail test function,\nthat lives in t/test-lib-functions.sh)\n\nThanks,\nStefan\n"},{"id":"339763","messageId":"xmqqinar1bq2.fsf@gitster-ct.c.googlers.com","threadId":"47876","inReplyTo":"CAGZ79kbXXN_nxFZUorsbFHcg4-8H7ZnAOnaViUrnP4r4u6ThhQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] tag: Make \"git tag --contains <id>\" less chatty if <id> is invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-20T23:29:41Z","receivedAt":"2018-02-20T23:29:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> diff --git a/t/t7013-tag-contains.sh b/t/t7013-tag-contains.sh\n>> new file mode 100755\n>> index 000000000..65119dada\n>> --- /dev/null\n>> +++ b/t/t7013-tag-contains.sh\n>\n> Thanks for adding the tests into a new file instead of putting it somewhere\n> where it is already convenient. (We have too many of those \"just add it there\n> as it is easiest to fit in\")\n\nCareful, as that cuts both ways.  We want to strongly encourage\npeople to see if there is already a place that is a good enough fit\nfor new tests before adding small test scripts randomly to consume\nthe test serial numbers and test process start-up cost.  Only when\nthere is nowhere appropriate, we do want to add.  And if this covers\nboth tag and branch, then a new script may be appropriate but it\nshouldn't limit its future enhancement (to test 'git branch') by\nhaving 'tag' to pretend that this file must be limited to 'git tag'.\n\n> So I'd expect the return code to be 0 (if we don't care) or 1\n> (if we do care), in the case of 1, we shall write:\n>\n>   test_must_fail git tag --contains ... &&\n>   grep ....\n>\n> (A long way of hinting at the test_must_fail test function,\n> that lives in t/test-lib-functions.sh)\n\nIf you are looking at error stream, it is very likely that you would\nwant to study test_i18ngrep and use it, as errors are fair game for\ni18n (and possibly coloring, which is a near-by topic).\n"}]}