{"thread":{"id":"59952","subject":"[PATCH 0/2] Add new \"describe\" atom","startedAt":"2023-07-05T18:00:20Z","lastAt":"2023-07-25T21:47:00Z","messageCount":38,"participants":["Kousik Sanagavarapu","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479218","messageId":"20230705175942.21090-1-five231003@gmail.com","threadId":"59952","inReplyTo":null,"subject":"[PATCH 0/2] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-05T17:57:10Z","receivedAt":"2023-07-05T18:00:20Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\nThis patch series focuses on duplicating the implementation of\n%(describe) and its friends from pretty to ref-filter, with the end goal\nof making ref-filter do everything that pretty is doing.\n\nPATCH 1/2 - This patch is the duplication of the placeholder from pretty\n\t ref-filter.\n\nPATCH 2/2 - This is an interesting case, where the tests written for the\n\t above duplication are successful but another test below, in\n\t t6300, \"Verify sorts with raw:size\" fails on linux-sha256 (CI).\n\nKousik Sanagavarapu (2):\n ref-filter: add new \"describe\" atom\n t6300: run describe atom tests on a different repo\n\n Documentation/git-for-each-ref.txt | 19 ++++\n ref-filter.c | 144 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 98 ++++++++++++++++++++\n 3 files changed, 261 insertions(+)\n\n-- \n2.41.0.237.g2d10a112d6.dirty\n\n"},{"id":"479219","messageId":"20230705175942.21090-2-five231003@gmail.com","threadId":"59952","inReplyTo":"20230705175942.21090-1-five231003@gmail.com","subject":"[PATCH 1/2] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-05T17:57:11Z","receivedAt":"2023-07-05T18:00:57Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Duplicate the logic of %(describe) and friends from pretty to\nref-filter. In the future, this change helps in unifying both the\nformats as ref-filter will be able to do everything that pretty is doing\nand we can have a single interface.\n\nThe new atom \"describe\" and its friends are equivalent to the existing\npretty formats with the same name.\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n Documentation/git-for-each-ref.txt | 19 ++++\n ref-filter.c | 144 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 85 +++++++++++++++++\n 3 files changed, 248 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 1e215d4e73..4ac5c3dac4 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -231,6 +231,25 @@ ahead-behind:::\n \tcommits ahead and behind, respectively, when comparing the output\n \tref to the `` specified in the format.\n \n+describe[:options]:: human-readable name, like\n+\t\t link-git:git-describe[1]; empty string for\n+\t\t undescribable commits. The `describe` string may be\n+\t\t followed by a colon and zero or more comma-separated\n+\t\t options. Descriptions can be inconsistent when tags\n+\t\t are added or removed at the same time.\n++\n+** tags=: Instead of only considering annotated tags, consider\n+\t\t lightweight tags as well.\n+** abbrev=: Instead of using the default number of hexadecimal digits\n+\t\t (which will vary according to the number of objects in the\n+\t\t repository with a default of 7) of the abbreviated\n+\t\t object name, use digits, or as many digits as\n+\t\t needed to form a unique object name.\n+** match=: Only consider tags matching the given `glob(7)` pattern,\n+\t\t excluding the \"refs/tags/\" prefix.\n+** exclude=: Do not consider tags matching the given `glob(7)`\n+\t\t pattern,excluding the \"refs/tags/\" prefix.\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e0d03a9f8e..6ec647c81f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -5,6 +5,7 @@\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"object-name.h\"\n@@ -146,6 +147,7 @@ enum atom_type {\n \tATOM_TAGGERDATE,\n \tATOM_CREATOR,\n \tATOM_CREATORDATE,\n+\tATOM_DESCRIBE,\n \tATOM_SUBJECT,\n \tATOM_BODY,\n \tATOM_TRAILERS,\n@@ -215,6 +217,13 @@ static struct used_atom {\n \t\tstruct email_option {\n \t\t\tenum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n \t\t} email_option;\n+\t\tstruct {\n+\t\t\tenum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,\n+\t\t\t D_MATCH } option;\n+\t\t\tunsigned int tagbool;\n+\t\t\tunsigned int length;\n+\t\t\tchar *pattern;\n+\t\t} describe;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \treturn 0;\n }\n \n+static int parse_describe_option(const char *arg)\n+{\n+\tif (!arg)\n+\t\treturn D_BARE;\n+\telse if (starts_with(arg, \"tags\"))\n+\t\treturn D_TAGS;\n+\telse if (starts_with(arg, \"abbrev\"))\n+\t\treturn D_ABBREV;\n+\telse if(starts_with(arg, \"exclude\"))\n+\t\treturn D_EXCLUDE;\n+\telse if (starts_with(arg, \"match\"))\n+\t\treturn D_MATCH;\n+\treturn -1;\n+}\n+\n+static int describe_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\tstruct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n+{\n+\tint opt = parse_describe_option(arg);\n+\n+\tswitch (opt) {\n+\tcase D_BARE:\n+\t\tbreak;\n+\tcase D_TAGS:\n+\t\t/*\n+\t\t * It is also possible to just use describe:tags, which\n+\t\t * is just treated as describe:tags=1\n+\t\t */\n+\t\tif (skip_prefix(arg, \"tags=\", &arg)) {\n+\t\t\tif (strtoul_ui(arg, 10, &atom->u.describe.tagbool))\n+\t\t\t\treturn strbuf_addf_ret(err, -1, _(\"boolean value \"\n+\t\t\t\t\t\t\"expected describe:tags=%s\"), arg);\n+\n+\t\t} else {\n+\t\t\tatom->u.describe.tagbool = 1;\n+\t\t}\n+\t\tbreak;\n+\tcase D_ABBREV:\n+\t\tskip_prefix(arg, \"abbrev=\", &arg);\n+\t\tif (strtoul_ui(arg, 10, &atom->u.describe.length))\n+\t\t\treturn strbuf_addf_ret(err, -1, _(\"positive value \"\n+\t\t\t\t\t \"expected describe:abbrev=%s\"), arg);\n+\t\tbreak;\n+\tcase D_EXCLUDE:\n+\t\tskip_prefix(arg, \"exclude=\", &arg);\n+\t\tatom->u.describe.pattern = xstrdup(arg);\n+\t\tbreak;\n+\tcase D_MATCH:\n+\t\tskip_prefix(arg, \"match=\", &arg);\n+\t\tatom->u.describe.pattern = xstrdup(arg);\n+\t\tbreak;\n+\tdefault:\n+\t\treturn err_bad_arg(err, \"describe\", arg);\n+\t\tbreak;\n+\t}\n+\tatom->u.describe.option = opt;\n+\treturn 0;\n+}\n+\n static int raw_atom_parser(struct ref_format *format UNUSED,\n \t\t\t struct used_atom *atom,\n \t\t\t const char *arg, struct strbuf *err)\n@@ -664,6 +733,7 @@ static struct {\n \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n+\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n@@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n \t}\n }\n \n+static void grab_describe_values(struct atom_value *val, int deref,\n+\t\t\t\t struct object *obj)\n+{\n+\tstruct commit *commit = (struct commit *)obj;\n+\tint i;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tconst char *name = atom->name;\n+\t\tstruct atom_value *v = &val[i];\n+\t\tint opt;\n+\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tif (!!deref != (*name == '*'))\n+\t\t\tcontinue;\n+\t\tif (deref)\n+\t\t\tname++;\n+\n+\t\tif (!skip_prefix(name, \"describe\", &name) ||\n+\t\t (*name && *name != ':'))\n+\t\t\t continue;\n+\t\tif (!*name)\n+\t\t\tname = NULL;\n+\t\telse\n+\t\t\tname++;\n+\n+\t\topt = parse_describe_option(name);\n+\t\tif (opt < 0)\n+\t\t\tcontinue;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_push(&cmd.args, \"describe\");\n+\n+\t\tswitch(opt) {\n+\t\tcase D_BARE:\n+\t\t\tbreak;\n+\t\tcase D_TAGS:\n+\t\t\tif (atom->u.describe.tagbool)\n+\t\t\t\tstrvec_push(&cmd.args, \"--tags\");\n+\t\t\telse\n+\t\t\t\tstrvec_push(&cmd.args, \"--no-tags\");\n+\t\t\tbreak;\n+\t\tcase D_ABBREV:\n+\t\t\tstrvec_pushf(&cmd.args, \"--abbrev=%d\",\n+\t\t\t\t atom->u.describe.length);\n+\t\t\tbreak;\n+\t\tcase D_EXCLUDE:\n+\t\t\tstrvec_pushf(&cmd.args, \"--exclude=%s\",\n+\t\t\t\t atom->u.describe.pattern);\n+\t\t\tbreak;\n+\t\tcase D_MATCH:\n+\t\t\tstrvec_pushf(&cmd.args, \"--match=%s\",\n+\t\t\t\t atom->u.describe.pattern);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n+\t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n+\t\t\terror(_(\"failed to run 'describe'\"));\n+\t\t\tv->s = xstrdup(\"\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_rtrim(&out);\n+\t\tv->s = strbuf_detach(&out, NULL);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)\n {\n@@ -1592,12 +1734,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_tag_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 5c00607608..98ea37d336 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n+test_expect_success 'describe atom vs git describe' '\n+\ttest_when_finished \"rm -rf describe-repo\" &&\n+\n+\tgit init describe-repo &&\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\ttest_commit --no-tag one &&\n+\t\tgit tag tagone &&\n+\n+\t\ttest_commit --no-tag two &&\n+\t\tgit tag -a -m \"tag two\" tagtwo &&\n+\n+\t\tgit for-each-ref refs/tags/ --format=\"%(objectname)\" >obj &&\n+\t\twhile read hash\n+\t\tdo\n+\t\t\tif desc=$(git describe $hash)\n+\t\t\tthen\n+\t\t\t\t: >expect-contains-good\n+\t\t\telse\n+\t\t\t\t: >expect-contains-bad\n+\t\t\tfi &&\n+\t\t\techo \"$hash $desc\" || return 1\n+\t\tdone expect &&\n+\t\ttest_path_exists expect-contains-good &&\n+\t\ttest_path_exists expect-contains-bad &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname) %(describe)\" \\\n+\t\t\trefs/tags/ >actual 2>err &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'describe:tags vs describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit for-each-ref --format=\"%(describe:tags)\" refs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\n+\t# Case 1: We have commits between HEAD and the most\n+\t# recent tag reachable from it\n+\ttest_commit --no-tag file &&\n+\tgit describe --abbrev=14 >expect &&\n+\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Make sure the hash used is atleast 14 digits long\n+\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\ttest 15 -le $(wc -c expect &&\n+\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual &&\n+\ttest tagname = $(cat actual)\n+'\n+\n+test_expect_success 'describe:match=... vs describe --match ...' '\n+\ttest_when_finished \"git tag -d tag-match\" &&\n+\tgit tag -a -m \"tag match\" tag-match &&\n+\tgit describe --match \"*-match\" >expect &&\n+\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n+\ttest_when_finished \"git tag -d tag-exclude\" &&\n+\tgit tag -a -m \"tag exclude\" tag-exclude &&\n+\tgit describe --exclude \"*-exclude\" >expect &&\n+\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expected <<\\EOF\n heads/main\n tags/main\n-- \n2.41.0.237.g2d10a112d6.dirty\n\n"},{"id":"479220","messageId":"20230705175942.21090-3-five231003@gmail.com","threadId":"59952","inReplyTo":"20230705175942.21090-1-five231003@gmail.com","subject":"[PATCH 2/2] t6300: run describe atom tests on a different repo","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-05T17:57:12Z","receivedAt":"2023-07-05T18:01:11Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The tests for \"describe\" atom and its friends currently run on the main\nrepo of t6300, expect for the test for \"bare describe\", which is run on\n\"describe-repo\".\n\nThings can get messy with the other tests when such changes to a repo\nare done (for example, a new commit or a tag is introduced), especially\nin t6300 where the tests depend on commits and tags.\n\nAn example for this can be seen in [1], where writing the tests the\ncurrent way fails the test \"Verify sorts with raw:size\" on linux-sha256.\nThis, at first glance, seems totally unrelated.\n\nDigging in a bit deeper, it is apparent that this behavior is because of\nthe changes in the repo introduced when writing the \"describe\" tests,\nwhich changes the raw:size of an object. Such a change in raw-size would\nhave been, however, small if we were dealing with SHA1, but since we are\ndealing with SHA256, the change in raw:size is so significant that it\nfails the above mentioned test.\n\nSo, run all the \"describe\" atom tests on \"describe-repo\", which doesn't\ninterfere with the main repo on which the tests in t6300 are run.\n\n[1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++-----------------\n 1 file changed, 57 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 98ea37d336..181b04e699 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n-test_expect_success 'describe atom vs git describe' '\n-\ttest_when_finished \"rm -rf describe-repo\" &&\n-\n+test_expect_success 'setup for describe atom tests' '\n \tgit init describe-repo &&\n \t(\n \t\tcd describe-repo &&\n@@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' '\n \t\tgit tag tagone &&\n \n \t\ttest_commit --no-tag two &&\n-\t\tgit tag -a -m \"tag two\" tagtwo &&\n+\t\tgit tag -a -m \"tag two\" tagtwo\n+\t)\n+'\n+\n+test_expect_success 'describe atom vs git describe' '\n+\t(\n+\t\tcd describe-repo &&\n \n-\t\tgit for-each-ref refs/tags/ --format=\"%(objectname)\" >obj &&\n+\t\tgit for-each-ref --format=\"%(objectname)\" \\\n+\t\t\trefs/tags/ >obj &&\n \t\twhile read hash\n \t\tdo\n \t\t\tif desc=$(git describe $hash)\n@@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' '\n '\n \n test_expect_success 'describe:tags vs describe --tags' '\n-\ttest_when_finished \"git tag -d tagname\" &&\n-\tgit tag tagname &&\n-\tgit describe --tags >expect &&\n-\tgit for-each-ref --format=\"%(describe:tags)\" refs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit describe --tags >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:tags)\" \\\n+\t\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n-\ttest_when_finished \"git tag -d tagname\" &&\n-\n-\t# Case 1: We have commits between HEAD and the most\n-\t# recent tag reachable from it\n-\ttest_commit --no-tag file &&\n-\tgit describe --abbrev=14 >expect &&\n-\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual &&\n-\n-\t# Make sure the hash used is atleast 14 digits long\n-\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n-\ttest 15 -le $(wc -c expect &&\n-\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual &&\n-\ttest tagname = $(cat actual)\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# Case 1: We have commits between HEAD and the most\n+\t\t#\t recent tag reachable from it\n+\t\ttest_commit --no-tag file &&\n+\t\tgit describe --abbrev=14 >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Make sure the hash used is atleast 14 digits long\n+\t\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\t\ttest 15 -le $(wc -c expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest tagname = $(cat actual)\n+\t)\n '\n \n test_expect_success 'describe:match=... vs describe --match ...' '\n-\ttest_when_finished \"git tag -d tag-match\" &&\n-\tgit tag -a -m \"tag match\" tag-match &&\n-\tgit describe --match \"*-match\" >expect &&\n-\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag match\" tag-match &&\n+\t\tgit describe --match \"*-match\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n-\ttest_when_finished \"git tag -d tag-exclude\" &&\n-\tgit tag -a -m \"tag exclude\" tag-exclude &&\n-\tgit describe --exclude \"*-exclude\" >expect &&\n-\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag exclude\" tag-exclude &&\n+\t\tgit describe --exclude \"*-exclude\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n cat >expected <<\\EOF\n-- \n2.41.0.237.g2d10a112d6.dirty\n\n"},{"id":"479257","messageId":"xmqqbkgpdljy.fsf@gitster.g","threadId":"59952","inReplyTo":"20230705175942.21090-2-five231003@gmail.com","subject":"Re: [PATCH 1/2] ref-filter: add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-06T16:58:09Z","receivedAt":"2023-07-06T16:58:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> +describe[:options]:: human-readable name, like\n> +\t\t link-git:git-describe[1]; empty string for\n> +\t\t undescribable commits. The `describe` string may be\n> +\t\t followed by a colon and zero or more comma-separated\n> +\t\t options. Descriptions can be inconsistent when tags\n> +\t\t are added or removed at the same time.\n> ++\n> +** tags=: Instead of only considering annotated tags, consider\n> +\t\t lightweight tags as well.\n> +** abbrev=: Instead of using the default number of hexadecimal digits\n> +\t\t (which will vary according to the number of objects in the\n> +\t\t repository with a default of 7) of the abbreviated\n> +\t\t object name, use digits, or as many digits as\n> +\t\t needed to form a unique object name.\n> +** match=: Only consider tags matching the given `glob(7)` pattern,\n> +\t\t excluding the \"refs/tags/\" prefix.\n> +** exclude=: Do not consider tags matching the given `glob(7)`\n> +\t\t pattern,excluding the \"refs/tags/\" prefix.\n\nYou are missing a SP after the comma in \"pattern,excluding\" above.\n\nThe above description is slightly different from what \"git describe\n--help\" has. If they are described differently on purpose (e.g. you\nmay have made \"%(describe:abbrev=0)\" not to show only the closest\ntag, unlike \"git describe --abbrev=0\"), the differences should be\nspelled out more explicitly. If the behaviours of the option here\nand the corresponding one there are meant to be the same, then\neither using exactly the same text, or a much abbreviated\ndescription with a note referring to the option description of \"git\ndescribe\", would help the readers better. E.g.\n\n abbrev=;; use at least hexadecimal digits; see\n the corresponding option in linkgit:git-describe[1] for details.\n\nwhich would make it clear that no behavioral differences are meant.\n\nThis new section becomes a part of an existing \"labeled list\"\n(asciidoctor calls the construct \"description list\"). Starting the\nnew heading this patch adds with 'describe[:options]::' makes sense.\nIt is in line with the existing text.\n\nI however think that the list of options is better done as a nested\ndescription list. Documentation/config/color.txt has an example you\ncan imitate. See how slots of color.grep. are described\nthere.\n\n https://docs.asciidoctor.org/asciidoc/latest/lists/description/\n https://asciidoc-py.github.io/userguide.html#_labeled_lists\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index e0d03a9f8e..6ec647c81f 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -5,6 +5,7 @@\n> #include \"gpg-interface.h\"\n> #include \"hex.h\"\n> #include \"parse-options.h\"\n> +#include \"run-command.h\"\n> #include \"refs.h\"\n> #include \"wildmatch.h\"\n> #include \"object-name.h\"\n> @@ -146,6 +147,7 @@ enum atom_type {\n> \tATOM_TAGGERDATE,\n> \tATOM_CREATOR,\n> \tATOM_CREATORDATE,\n> +\tATOM_DESCRIBE,\n> \tATOM_SUBJECT,\n> \tATOM_BODY,\n> \tATOM_TRAILERS,\n> @@ -215,6 +217,13 @@ static struct used_atom {\n> \t\tstruct email_option {\n> \t\t\tenum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n> \t\t} email_option;\n> +\t\tstruct {\n> +\t\t\tenum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,\n> +\t\t\t D_MATCH } option;\n> +\t\t\tunsigned int tagbool;\n> +\t\t\tunsigned int length;\n\nThe name \"tagbool\" sounds strange, as we are not saying\n\"lengthint\".\n\n> +\t\t\tchar *pattern;\n> +\t\t} describe;\n\nI am a bit confused by this structure, actually, as I cannot quite\nguess from the data structure alone how you intend to use it. Does\nthis give a good representation for the piece of data you are trying\nto capture?\n\nFor example, %(describe:tags=no,abbrev=4) would become a single atom\nwith 0 in .tagbool and 4 in .length, but what does the .option\nmember get?\n\n> @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n> \treturn 0;\n> }\n> \n> +static int parse_describe_option(const char *arg)\n> +{\n> +\tif (!arg)\n> +\t\treturn D_BARE;\n> +\telse if (starts_with(arg, \"tags\"))\n> +\t\treturn D_TAGS;\n> +\telse if (starts_with(arg, \"abbrev\"))\n> +\t\treturn D_ABBREV;\n> +\telse if(starts_with(arg, \"exclude\"))\n> +\t\treturn D_EXCLUDE;\n> +\telse if (starts_with(arg, \"match\"))\n> +\t\treturn D_MATCH;\n> +\treturn -1;\n> +}\n> +\n> +static int describe_atom_parser(struct ref_format *format UNUSED,\n> +\t\t\t\tstruct used_atom *atom,\n> +\t\t\t\tconst char *arg, struct strbuf *err)\n> +{\n> +\tint opt = parse_describe_option(arg);\n> +\n> +\tswitch (opt) {\n> +\tcase D_BARE:\n> +\t\tbreak;\n> +\tcase D_TAGS:\n> +\t\t/*\n> +\t\t * It is also possible to just use describe:tags, which\n> +\t\t * is just treated as describe:tags=1\n> +\t\t */\n> +\t\tif (skip_prefix(arg, \"tags=\", &arg)) {\n> +\t\t\tif (strtoul_ui(arg, 10, &atom->u.describe.tagbool))\n\nThis is not how you accept a Boolean.\n\n\"1\", \"0\", \"yes\", \"no\", \"true\", \"false\", \"on\", \"off\" are all valid\nvalues and you use git_parse_maybe_bool() to parse them.\n\n> +\t\t\t\treturn strbuf_addf_ret(err, -1, _(\"boolean value \"\n> +\t\t\t\t\t\t\"expected describe:tags=%s\"), arg);\n> +\n> +\t\t} else {\n> +\t\t\tatom->u.describe.tagbool = 1;\n> +\t\t}\n> +\t\tbreak;\n> +\tcase D_ABBREV:\n> +\t\tskip_prefix(arg, \"abbrev=\", &arg);\n> +\t\tif (strtoul_ui(arg, 10, &atom->u.describe.length))\n> +\t\t\treturn strbuf_addf_ret(err, -1, _(\"positive value \"\n> +\t\t\t\t\t \"expected describe:abbrev=%s\"), arg);\n> +\t\tbreak;\n> +\tcase D_EXCLUDE:\n> +\t\tskip_prefix(arg, \"exclude=\", &arg);\n> +\t\tatom->u.describe.pattern = xstrdup(arg);\n> +\t\tbreak;\n> +\tcase D_MATCH:\n> +\t\tskip_prefix(arg, \"match=\", &arg);\n> +\t\tatom->u.describe.pattern = xstrdup(arg);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn err_bad_arg(err, \"describe\", arg);\n> +\t\tbreak;\n> +\t}\n> +\tatom->u.describe.option = opt;\n> +\treturn 0;\n> +}\n\nEven though the documentation patch we saw earlier said \"may be\nfollowed by a colon and zero or more comma-separated options\", this\nseems to expect only and exactly one option. Indeed, if we run the\nresulting git like \"git for-each-ref --format='%(describe:tags=0,abbrev=4)'\"\nyou will get complaints from this parser.\n\nThe implementation needs redesigning as the data structure is not\nequipped to handle more than one options given at the same time, as\nwe saw earlier.\n\n> @@ -664,6 +733,7 @@ static struct {\n> \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n> \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n> \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n> +\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n> \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n> \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n> \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n> @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n> \t}\n> }\n> \n> +static void grab_describe_values(struct atom_value *val, int deref,\n> +\t\t\t\t struct object *obj)\n> +{\n> +\tstruct commit *commit = (struct commit *)obj;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < used_atom_cnt; i++) {\n> +\t\tstruct used_atom *atom = &used_atom[i];\n> +\t\tconst char *name = atom->name;\n> +\t\tstruct atom_value *v = &val[i];\n> +\t\tint opt;\n> +\n> +\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\t\tif (!!deref != (*name == '*'))\n> +\t\t\tcontinue;\n> +\t\tif (deref)\n> +\t\t\tname++;\n> +\n> +\t\tif (!skip_prefix(name, \"describe\", &name) ||\n> +\t\t (*name && *name != ':'))\n> +\t\t\t continue;\n\nThis looks overly expensive. Why aren't we looking at the atom_type\nand see if it is ATOM_DESCRIBE here?\n\n> +\t\tswitch(opt) {\n> +\t\tcase D_BARE:\n> +\t\t\tbreak;\n> +\t\tcase D_TAGS:\n> +\t\t\tif (atom->u.describe.tagbool)\n> +\t\t\t\tstrvec_push(&cmd.args, \"--tags\");\n> +\t\t\telse\n> +\t\t\t\tstrvec_push(&cmd.args, \"--no-tags\");\n> +\t\t\tbreak;\n> +\t\tcase D_ABBREV:\n> +\t\t\tstrvec_pushf(&cmd.args, \"--abbrev=%d\",\n> +\t\t\t\t atom->u.describe.length);\n> +\t\t\tbreak;\n> +\t\tcase D_EXCLUDE:\n> +\t\t\tstrvec_pushf(&cmd.args, \"--exclude=%s\",\n> +\t\t\t\t atom->u.describe.pattern);\n> +\t\t\tbreak;\n> +\t\tcase D_MATCH:\n> +\t\t\tstrvec_pushf(&cmd.args, \"--match=%s\",\n> +\t\t\t\t atom->u.describe.pattern);\n> +\t\t\tbreak;\n> +\t\t}\n\nAgain, it is apparent here that the atom takes only one option at most.\n\nI'll stop here.\n\nThanks.\n"},{"id":"479339","messageId":"ZKpQo7WdQXk51MdN@five231003","threadId":"59952","inReplyTo":"xmqqbkgpdljy.fsf@gitster.g","subject":"Re: [PATCH 1/2] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-09T06:16:03Z","receivedAt":"2023-07-09T06:16:25Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Thu, Jul 06, 2023 at 09:58:09AM -0700, Junio C Hamano wrote:\n> Kousik Sanagavarapu writes:\n> \n> > +describe[:options]:: human-readable name, like\n> > +\t\t link-git:git-describe[1]; empty string for\n> > +\t\t undescribable commits. The `describe` string may be\n> > +\t\t followed by a colon and zero or more comma-separated\n> > +\t\t options. Descriptions can be inconsistent when tags\n> > +\t\t are added or removed at the same time.\n> > ++\n> > +** tags=: Instead of only considering annotated tags, consider\n> > +\t\t lightweight tags as well.\n> > +** abbrev=: Instead of using the default number of hexadecimal digits\n> > +\t\t (which will vary according to the number of objects in the\n> > +\t\t repository with a default of 7) of the abbreviated\n> > +\t\t object name, use digits, or as many digits as\n> > +\t\t needed to form a unique object name.\n> > +** match=: Only consider tags matching the given `glob(7)` pattern,\n> > +\t\t excluding the \"refs/tags/\" prefix.\n> > +** exclude=: Do not consider tags matching the given `glob(7)`\n> > +\t\t pattern,excluding the \"refs/tags/\" prefix.\n> \n> You are missing a SP after the comma in \"pattern,excluding\" above.\n> \n> The above description is slightly different from what \"git describe\n> --help\" has. If they are described differently on purpose (e.g. you\n> may have made \"%(describe:abbrev=0)\" not to show only the closest\n> tag, unlike \"git describe --abbrev=0\"), the differences should be\n> spelled out more explicitly. If the behaviours of the option here\n> and the corresponding one there are meant to be the same, then\n> either using exactly the same text, or a much abbreviated\n> description with a note referring to the option description of \"git\n> describe\", would help the readers better. E.g.\n> \n> abbrev=;; use at least hexadecimal digits; see\n> the corresponding option in linkgit:git-describe[1] for details.\n> \n> which would make it clear that no behavioral differences are meant.\n> \n> This new section becomes a part of an existing \"labeled list\"\n> (asciidoctor calls the construct \"description list\"). Starting the\n> new heading this patch adds with 'describe[:options]::' makes sense.\n> It is in line with the existing text.\n> \n> I however think that the list of options is better done as a nested\n> description list. Documentation/config/color.txt has an example you\n> can imitate. See how slots of color.grep. are described\n> there.\n> \n> https://docs.asciidoctor.org/asciidoc/latest/lists/description/\n> https://asciidoc-py.github.io/userguide.html#_labeled_lists\n\nI read through these and looked at the examples (on the git-scm website\nwhere this is used as well). I'll make the changes. Thanks for this.\n\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index e0d03a9f8e..6ec647c81f 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -5,6 +5,7 @@\n> > #include \"gpg-interface.h\"\n> > #include \"hex.h\"\n> > #include \"parse-options.h\"\n> > +#include \"run-command.h\"\n> > #include \"refs.h\"\n> > #include \"wildmatch.h\"\n> > #include \"object-name.h\"\n> > @@ -146,6 +147,7 @@ enum atom_type {\n> > \tATOM_TAGGERDATE,\n> > \tATOM_CREATOR,\n> > \tATOM_CREATORDATE,\n> > +\tATOM_DESCRIBE,\n> > \tATOM_SUBJECT,\n> > \tATOM_BODY,\n> > \tATOM_TRAILERS,\n> > @@ -215,6 +217,13 @@ static struct used_atom {\n> > \t\tstruct email_option {\n> > \t\t\tenum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n> > \t\t} email_option;\n> > +\t\tstruct {\n> > +\t\t\tenum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,\n> > +\t\t\t D_MATCH } option;\n> > +\t\t\tunsigned int tagbool;\n> > +\t\t\tunsigned int length;\n> \n> The name \"tagbool\" sounds strange, as we are not saying\n> \"lengthint\".\n\nYeah, it does sound strange now that you take the example of \"lenghtint\".\nI'm thinking \"use_tags\" might be good, but I don't know.\n\n> > +\t\t\tchar *pattern;\n> > +\t\t} describe;\n> \n> I am a bit confused by this structure, actually, as I cannot quite\n> guess from the data structure alone how you intend to use it. Does\n> this give a good representation for the piece of data you are trying\n> to capture?\n> \n> For example, %(describe:tags=no,abbrev=4) would become a single atom\n> with 0 in .tagbool and 4 in .length, but what does the .option\n> member get?\n\nThat is true. After I read your email, I started thinking of redesigning\nit. My initial thought on doing this was to mimic the other atoms'\ndesign in \"struct used_atom\" but now that I have read your email, it\nseems that such a design doesn't work. Maybe I could do something like\nhow it is handled in pretty while also not losing the design of other\natoms in \"struct used_atom\".\n\n> > @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n> > \treturn 0;\n> > }\n> > \n> > +static int parse_describe_option(const char *arg)\n> > +{\n> > +\tif (!arg)\n> > +\t\treturn D_BARE;\n> > +\telse if (starts_with(arg, \"tags\"))\n> > +\t\treturn D_TAGS;\n> > +\telse if (starts_with(arg, \"abbrev\"))\n> > +\t\treturn D_ABBREV;\n> > +\telse if(starts_with(arg, \"exclude\"))\n> > +\t\treturn D_EXCLUDE;\n> > +\telse if (starts_with(arg, \"match\"))\n> > +\t\treturn D_MATCH;\n> > +\treturn -1;\n> > +}\n> > +\n> > +static int describe_atom_parser(struct ref_format *format UNUSED,\n> > +\t\t\t\tstruct used_atom *atom,\n> > +\t\t\t\tconst char *arg, struct strbuf *err)\n> > +{\n> > +\tint opt = parse_describe_option(arg);\n> > +\n> > +\tswitch (opt) {\n> > +\tcase D_BARE:\n> > +\t\tbreak;\n> > +\tcase D_TAGS:\n> > +\t\t/*\n> > +\t\t * It is also possible to just use describe:tags, which\n> > +\t\t * is just treated as describe:tags=1\n> > +\t\t */\n> > +\t\tif (skip_prefix(arg, \"tags=\", &arg)) {\n> > +\t\t\tif (strtoul_ui(arg, 10, &atom->u.describe.tagbool))\n> \n> This is not how you accept a Boolean.\n> \n> \"1\", \"0\", \"yes\", \"no\", \"true\", \"false\", \"on\", \"off\" are all valid\n> values and you use git_parse_maybe_bool() to parse them.\n\nWill make this change.\n\n> > +\t\t\t\treturn strbuf_addf_ret(err, -1, _(\"boolean value \"\n> > +\t\t\t\t\t\t\"expected describe:tags=%s\"), arg);\n> > +\n> > +\t\t} else {\n> > +\t\t\tatom->u.describe.tagbool = 1;\n> > +\t\t}\n> > +\t\tbreak;\n> > +\tcase D_ABBREV:\n> > +\t\tskip_prefix(arg, \"abbrev=\", &arg);\n> > +\t\tif (strtoul_ui(arg, 10, &atom->u.describe.length))\n> > +\t\t\treturn strbuf_addf_ret(err, -1, _(\"positive value \"\n> > +\t\t\t\t\t \"expected describe:abbrev=%s\"), arg);\n> > +\t\tbreak;\n> > +\tcase D_EXCLUDE:\n> > +\t\tskip_prefix(arg, \"exclude=\", &arg);\n> > +\t\tatom->u.describe.pattern = xstrdup(arg);\n> > +\t\tbreak;\n> > +\tcase D_MATCH:\n> > +\t\tskip_prefix(arg, \"match=\", &arg);\n> > +\t\tatom->u.describe.pattern = xstrdup(arg);\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\treturn err_bad_arg(err, \"describe\", arg);\n> > +\t\tbreak;\n> > +\t}\n> > +\tatom->u.describe.option = opt;\n> > +\treturn 0;\n> > +}\n> \n> Even though the documentation patch we saw earlier said \"may be\n> followed by a colon and zero or more comma-separated options\", this\n> seems to expect only and exactly one option. Indeed, if we run the\n> resulting git like \"git for-each-ref --format='%(describe:tags=0,abbrev=4)'\"\n> you will get complaints from this parser.\n> \n> The implementation needs redesigning as the data structure is not\n> equipped to handle more than one options given at the same time, as\n> we saw earlier.\n> \n> > @@ -664,6 +733,7 @@ static struct {\n> > \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n> > \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n> > \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n> > +\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n> > \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n> > \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n> > \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n> > @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n> > \t}\n> > }\n> > \n> > +static void grab_describe_values(struct atom_value *val, int deref,\n> > +\t\t\t\t struct object *obj)\n> > +{\n> > +\tstruct commit *commit = (struct commit *)obj;\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < used_atom_cnt; i++) {\n> > +\t\tstruct used_atom *atom = &used_atom[i];\n> > +\t\tconst char *name = atom->name;\n> > +\t\tstruct atom_value *v = &val[i];\n> > +\t\tint opt;\n> > +\n> > +\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\t\tstruct strbuf out = STRBUF_INIT;\n> > +\t\tstruct strbuf err = STRBUF_INIT;\n> > +\n> > +\t\tif (!!deref != (*name == '*'))\n> > +\t\t\tcontinue;\n> > +\t\tif (deref)\n> > +\t\t\tname++;\n> > +\n> > +\t\tif (!skip_prefix(name, \"describe\", &name) ||\n> > +\t\t (*name && *name != ':'))\n> > +\t\t\t continue;\n> \n> This looks overly expensive. Why aren't we looking at the atom_type\n> and see if it is ATOM_DESCRIBE here?\n> \n> > +\t\tswitch(opt) {\n> > +\t\tcase D_BARE:\n> > +\t\t\tbreak;\n> > +\t\tcase D_TAGS:\n> > +\t\t\tif (atom->u.describe.tagbool)\n> > +\t\t\t\tstrvec_push(&cmd.args, \"--tags\");\n> > +\t\t\telse\n> > +\t\t\t\tstrvec_push(&cmd.args, \"--no-tags\");\n> > +\t\t\tbreak;\n> > +\t\tcase D_ABBREV:\n> > +\t\t\tstrvec_pushf(&cmd.args, \"--abbrev=%d\",\n> > +\t\t\t\t atom->u.describe.length);\n> > +\t\t\tbreak;\n> > +\t\tcase D_EXCLUDE:\n> > +\t\t\tstrvec_pushf(&cmd.args, \"--exclude=%s\",\n> > +\t\t\t\t atom->u.describe.pattern);\n> > +\t\t\tbreak;\n> > +\t\tcase D_MATCH:\n> > +\t\t\tstrvec_pushf(&cmd.args, \"--match=%s\",\n> > +\t\t\t\t atom->u.describe.pattern);\n> > +\t\t\tbreak;\n> > +\t\t}\n> \n> Again, it is apparent here that the atom takes only one option at most.\n\nYeah. I'll reroll with the necessary changes so that we handle many\noptions like the Documentation patch says and also the other things you\npointed out.\n\nThanks\n"},{"id":"479529","messageId":"20230714194249.66862-1-five231003@gmail.com","threadId":"59952","inReplyTo":"20230705175942.21090-1-five231003@gmail.com","subject":"[PATCH v2 0/3] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-14T19:20:25Z","receivedAt":"2023-07-14T19:43:19Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\nThis patch series addresses the previous comments and so now we can do\nfor example\n\n\tgit for-each-ref --format=\"%(describe:tags=yes,abbrev=14)\"\n\nPATCH 1/3 - This is a new commit which introduces two new functions for\n\t handling multiple options in ref-filter.\n\n\t There are two ways to do this\n\t - We change the functions in pretty so that they can be used\n\t generally and not only for placeholders.\n\t - We introduce corresponding functions in ref-filter for\n\t handling atoms.\n\n\t This patch follows the second approach but the first\n\t approach is also good because we don't duplicate the code.\n\t Or maybe there is a much better approach that I don't see.\n\nPATCH 2/3 - Changes are made so that we can handle multiple options and\n\t also the related docs are a nested description list.\n\nPATCH 3/3 - This commit is left unchanged.\n\nKousik Sanagavarapu (3):\n ref-filter: add multiple-option parsing functions\n ref-filter: add new \"describe\" atom\n t6300: run describe atom tests on a different repo\n\n Documentation/git-for-each-ref.txt | 23 ++++\n ref-filter.c | 206 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 98 ++++++++++++++\n 3 files changed, 327 insertions(+)\n\nRange-diff against v1:\n\n-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing\nfunctions\n1: 9e3e652659 ! 2: f6f882884c ref-filter: add new \"describe\" atom\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n commits ahead and behind, respectively, when comparing the\noutput\n ref to the `` specified in the format.\n \n -+describe[:options]:: human-readable name, like\n ++describe[:options]:: Human-readable name, like\n + link-git:git-describe[1]; empty string for\n + undescribable commits. The `describe` string may be\n + followed by a colon and zero or more\ncomma-separated\n + options. Descriptions can be inconsistent when tags\n + are added or removed at the same time.\n ++\n -+** tags=: Instead of only considering annotated tags,\nconsider\n -+ lightweight tags as well.\n -+** abbrev=: Instead of using the default number of\nhexadecimal digits\n -+ (which will vary according to the number of objects\nin the\n -+ repository with a default of 7) of the abbreviated\n -+ object name, use digits, or as many digits\nas\n -+ needed to form a unique object name.\n -+** match=: Only consider tags matching the given\n`glob(7)` pattern,\n -+ excluding the \"refs/tags/\" prefix.\n -+** exclude=: Do not consider tags matching the given\n`glob(7)`\n -+ pattern,excluding the \"refs/tags/\" prefix.\n ++--\n ++tags=;; Instead of only considering annotated tags,\nconsider\n ++ lightweight tags as well; see the corresponding\noption\n ++ in linkgit:git-describe[1] for details.\n ++abbrev=;; Use at least hexadecimal digits; see\n ++ the corresponding option in linkgit:git-describe[1]\n ++ for details.\n ++match=;; Only consider tags matching the given `glob(7)`\npattern,\n ++ excluding the \"refs/tags/\" prefix; see the\ncorresponding\n ++ option in linkgit:git-describe[1] for details.\n ++exclude=;; Do not consider tags matching the given\n`glob(7)`\n ++ pattern, excluding the \"refs/tags/\" prefix; see the\n ++ corresponding option in linkgit:git-describe[1] for\n ++ details.\n ++--\n +\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n \n ## ref-filter.c ##\n @@\n + #include \"alloc.h\"\n + #include \"environment.h\"\n + #include \"gettext.h\"\n ++#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n @@ ref-filter.c: static struct used_atom {\n enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n } email_option;\n + struct {\n -+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,\n -+ D_MATCH } option;\n -+ unsigned int tagbool;\n -+ unsigned int length;\n -+ char *pattern;\n ++ enum { D_BARE, D_TAGS, D_ABBREV,\n ++ D_EXCLUDE, D_MATCH } option;\n ++ const char **args;\n + } describe;\n struct refname_atom refname;\n char *head;\n @@ ref-filter.c: static int contents_atom_parser(struct ref_format\n*format, struct\n return 0;\n }\n \n -+static int parse_describe_option(const char *arg)\n -+{\n -+ if (!arg)\n -+ return D_BARE;\n -+ else if (starts_with(arg, \"tags\"))\n -+ return D_TAGS;\n -+ else if (starts_with(arg, \"abbrev\"))\n -+ return D_ABBREV;\n -+ else if(starts_with(arg, \"exclude\"))\n -+ return D_EXCLUDE;\n -+ else if (starts_with(arg, \"match\"))\n -+ return D_MATCH;\n -+ return -1;\n -+}\n -+\n +static int describe_atom_parser(struct ref_format *format UNUSED,\n + struct used_atom *atom,\n + const char *arg, struct strbuf *err)\n +{\n -+ int opt = parse_describe_option(arg);\n ++ const char *describe_opts[] = {\n ++ \"\",\n ++ \"tags\",\n ++ \"abbrev\",\n ++ \"match\",\n ++ \"exclude\",\n ++ NULL\n ++ };\n ++\n ++ struct strvec args = STRVEC_INIT;\n ++ for (;;) {\n ++ int found = 0;\n ++ const char *argval;\n ++ size_t arglen = 0;\n ++ int optval = 0;\n ++ int opt;\n ++\n ++ if (!arg)\n ++ break;\n ++\n ++ for (opt = D_BARE; !found && describe_opts[opt]; opt++)\n{\n ++ switch(opt) {\n ++ case D_BARE:\n ++ /*\n ++ * Do nothing. This is the bare describe\n ++ * atom and we already handle this\nabove.\n ++ */\n ++ break;\n ++ case D_TAGS:\n ++ if (match_atom_bool_arg(arg,\ndescribe_opts[opt],\n ++ &arg, &optval))\n{\n ++ if (!optval)\n ++ strvec_pushf(&args,\n\"--no-%s\",\n ++\ndescribe_opts[opt]);\n ++ else\n ++ strvec_pushf(&args,\n\"--%s\",\n ++\ndescribe_opts[opt]);\n ++ found = 1;\n ++ }\n ++ break;\n ++ case D_ABBREV:\n ++ if (match_atom_arg_value(arg,\ndescribe_opts[opt],\n ++ &arg, &argval,\n&arglen)) {\n ++ char *endptr;\n ++ int ret = 0;\n +\n -+ switch (opt) {\n -+ case D_BARE:\n -+ break;\n -+ case D_TAGS:\n -+ /*\n -+ * It is also possible to just use describe:tags, which\n -+ * is just treated as describe:tags=1\n -+ */\n -+ if (skip_prefix(arg, \"tags=\", &arg)) {\n -+ if (strtoul_ui(arg, 10,\n&atom->u.describe.tagbool))\n -+ return strbuf_addf_ret(err, -1,\n_(\"boolean value \"\n -+ \"expected\ndescribe:tags=%s\"), arg);\n ++ if (!arglen)\n ++ ret = -1;\n ++ if (strtol(argval, &endptr, 10)\n< 0)\n ++ ret = -1;\n ++ if (endptr - argval != arglen)\n ++ ret = -1;\n +\n -+ } else {\n -+ atom->u.describe.tagbool = 1;\n ++ if (ret)\n ++ return\nstrbuf_addf_ret(err, ret,\n ++\n_(\"positive value expected describe:abbrev=%s\"), argval);\n ++ strvec_pushf(&args, \"--%s=%.*s\",\n ++ describe_opts[opt],\n ++ (int)arglen,\nargval);\n ++ found = 1;\n ++ }\n ++ break;\n ++ case D_MATCH:\n ++ case D_EXCLUDE:\n ++ if (match_atom_arg_value(arg,\ndescribe_opts[opt],\n ++ &arg, &argval,\n&arglen)) {\n ++ if (!arglen)\n ++ return\nstrbuf_addf_ret(err, -1,\n ++ _(\"value\nexpected describe:%s=\"), describe_opts[opt]);\n ++ strvec_pushf(&args, \"--%s=%.*s\",\n ++ describe_opts[opt],\n ++ (int)arglen,\nargval);\n ++ found = 1;\n ++ }\n ++ break;\n ++ }\n + }\n -+ break;\n -+ case D_ABBREV:\n -+ skip_prefix(arg, \"abbrev=\", &arg);\n -+ if (strtoul_ui(arg, 10, &atom->u.describe.length))\n -+ return strbuf_addf_ret(err, -1, _(\"positive\nvalue \"\n -+ \"expected\ndescribe:abbrev=%s\"), arg);\n -+ break;\n -+ case D_EXCLUDE:\n -+ skip_prefix(arg, \"exclude=\", &arg);\n -+ atom->u.describe.pattern = xstrdup(arg);\n -+ break;\n -+ case D_MATCH:\n -+ skip_prefix(arg, \"match=\", &arg);\n -+ atom->u.describe.pattern = xstrdup(arg);\n -+ break;\n -+ default:\n -+ return err_bad_arg(err, \"describe\", arg);\n -+ break;\n ++ if (!found)\n ++ break;\n + }\n -+ atom->u.describe.option = opt;\n ++ atom->u.describe.args = strvec_detach(&args);\n + return 0;\n +}\n +\n @@ ref-filter.c: static void append_lines(struct strbuf *out, const\nchar *buf, unsi\n +\n + for (i = 0; i < used_atom_cnt; i++) {\n + struct used_atom *atom = &used_atom[i];\n ++ enum atom_type type = atom->atom_type;\n + const char *name = atom->name;\n + struct atom_value *v = &val[i];\n -+ int opt;\n +\n + struct child_process cmd = CHILD_PROCESS_INIT;\n + struct strbuf out = STRBUF_INIT;\n + struct strbuf err = STRBUF_INIT;\n +\n ++ if (type != ATOM_DESCRIBE)\n ++ continue;\n ++\n + if (!!deref != (*name == '*'))\n + continue;\n + if (deref)\n @@ ref-filter.c: static void append_lines(struct strbuf *out, const\nchar *buf, unsi\n + else\n + name++;\n +\n -+ opt = parse_describe_option(name);\n -+ if (opt < 0)\n -+ continue;\n -+\n + cmd.git_cmd = 1;\n + strvec_push(&cmd.args, \"describe\");\n -+\n -+ switch(opt) {\n-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing\nfunctions\n1: 9e3e652659 ! 2: f6f882884c ref-filter: add new \"describe\" atom\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n commits ahead and behind, respectively, when comparing the\noutput\n ref to the `` specified in the format.\n \n -+describe[:options]:: human-readable name, like\n ++describe[:options]:: Human-readable name, like\n + link-git:git-describe[1]; empty string for\n + undescribable commits. The `describe` string may be\n + followed by a colon and zero or more\ncomma-separated\n + options. Descriptions can be inconsistent when tags\n + are added or removed at the same time.\n ++\n -+** tags=: Instead of only considering annotated tags,\nconsider\n -+ lightweight tags as well.\n -+** abbrev=: Instead of using the default number of\nhexadecimal digits\n -+ (which will vary according to the number of objects\nin the\n -+ repository with a default of 7) of the abbreviated\n -+ object name, use digits, or as many digits\nas\n -+ needed to form a unique object name.\n -+** match=: Only consider tags matching the given\n`glob(7)` pattern,\n -+ excluding the \"refs/tags/\" prefix.\n -+** exclude=: Do not consider tags matching the given\n`glob(7)`\n -+ pattern,excluding the \"refs/tags/\" prefix.\n ++--\n ++tags=;; Instead of only considering annotated tags,\nconsider\n ++ lightweight tags as well; see the corresponding\noption\n ++ in linkgit:git-describe[1] for details.\n ++abbrev=;; Use at least hexadecimal digits; see\n ++ the corresponding option in linkgit:git-describe[1]\n ++ for details.\n ++match=;; Only consider tags matching the given `glob(7)`\npattern,\n ++ excluding the \"refs/tags/\" prefix; see the\ncorresponding\n ++ option in linkgit:git-describe[1] for details.\n ++exclude=;; Do not consider tags matching the given\n`glob(7)`\n ++ pattern, excluding the \"refs/tags/\" prefix; see the\n ++ corresponding option in linkgit:git-describe[1] for\n ++ details.\n ++--\n +\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n \n ## ref-filter.c ##\n @@\n + #include \"alloc.h\"\n + #include \"environment.h\"\n + #include \"gettext.h\"\n ++#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n @@ ref-filter.c: static struct used_atom {\n enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n } email_option;\n + struct {\n -+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE,\n -+ D_MATCH } option;\n -+ unsigned int tagbool;\n -+ unsigned int length;\n -+ char *pattern;\n ++ enum { D_BARE, D_TAGS, D_ABBREV,\n ++ D_EXCLUDE, D_MATCH } option;\n ++ const char **args;\n + } describe;\n struct refname_atom refname;\n char *head;\n @@ ref-filter.c: static int contents_atom_parser(struct ref_format\n*format, struct\n return 0;\n }\n \n -+static int parse_describe_option(const char *arg)\n -+{\n -+ if (!arg)\n -+ return D_BARE;\n -+ else if (starts_with(arg, \"tags\"))\n -+ return D_TAGS;\n -+ else if (starts_with(arg, \"abbrev\"))\n -+ return D_ABBREV;\n -+ else if(starts_with(arg, \"exclude\"))\n -+ return D_EXCLUDE;\n -+ else if (starts_with(arg, \"match\"))\n -+ return D_MATCH;\n -+ return -1;\n -+}\n -+\n +static int describe_atom_parser(struct ref_format *format UNUSED,\n + struct used_atom *atom,\n + const char *arg, struct strbuf *err)\n +{\n -+ int opt = parse_describe_option(arg);\n ++ const char *describe_opts[] = {\n ++ \"\",\n ++ \"tags\",\n ++ \"abbrev\",\n ++ \"match\",\n ++ \"exclude\",\n ++ NULL\n ++ };\n ++\n ++ struct strvec args = STRVEC_INIT;\n ++ for (;;) {\n ++ int found = 0;\n ++ const char *argval;\n ++ size_t arglen = 0;\n ++ int optval = 0;\n ++ int opt;\n ++\n ++ if (!arg)\n ++ break;\n ++\n ++ for (opt = D_BARE; !found && describe_opts[opt]; opt++)\n{\n ++ switch(opt) {\n ++ case D_BARE:\n ++ /*\n ++ * Do nothing. This is the bare describe\n ++ * atom and we already handle this\nabove.\n ++ */\n ++ break;\n ++ case D_TAGS:\n ++ if (match_atom_bool_arg(arg,\ndescribe_opts[opt],\n ++ &arg, &optval))\n{\n ++ if (!optval)\n ++ strvec_pushf(&args,\n\"--no-%s\",\n ++\ndescribe_opts[opt]);\n ++ else\n ++ strvec_pushf(&args,\n\"--%s\",\n ++\ndescribe_opts[opt]);\n ++ found = 1;\n ++ }\n ++ break;\n ++ case D_ABBREV:\n ++ if (match_atom_arg_value(arg,\ndescribe_opts[opt],\n ++ &arg, &argval,\n&arglen)) {\n ++ char *endptr;\n ++ int ret = 0;\n +\n -+ switch (opt) {\n -+ case D_BARE:\n -+ break;\n -+ case D_TAGS:\n -+ /*\n -+ * It is also possible to just use describe:tags, which\n -+ * is just treated as describe:tags=1\n -+ */\n -+ if (skip_prefix(arg, \"tags=\", &arg)) {\n -+ if (strtoul_ui(arg, 10,\n&atom->u.describe.tagbool))\n -+ return strbuf_addf_ret(err, -1,\n_(\"boolean value \"\n -+ \"expected\ndescribe:tags=%s\"), arg);\n ++ if (!arglen)\n ++ ret = -1;\n ++ if (strtol(argval, &endptr, 10)\n< 0)\n ++ ret = -1;\n ++ if (endptr - argval != arglen)\n ++ ret = -1;\n +\n -+ } else {\n -+ atom->u.describe.tagbool = 1;\n ++ if (ret)\n ++ return\nstrbuf_addf_ret(err, ret,\n ++\n_(\"positive value expected describe:abbrev=%s\"), argval);\n ++ strvec_pushf(&args, \"--%s=%.*s\",\n ++ describe_opts[opt],\n ++ (int)arglen,\nargval);\n ++ found = 1;\n ++ }\n ++ break;\n ++ case D_MATCH:\n ++ case D_EXCLUDE:\n ++ if (match_atom_arg_value(arg,\ndescribe_opts[opt],\n ++ &arg, &argval,\n&arglen)) {\n ++ if (!arglen)\n ++ return\nstrbuf_addf_ret(err, -1,\n...skipping...\n -+ case D_BARE:\n -+ break;\n -+ case D_TAGS:\n -+ if (atom->u.describe.tagbool)\n -+ strvec_push(&cmd.args, \"--tags\");\n -+ else\n -+ strvec_push(&cmd.args, \"--no-tags\");\n -+ break;\n -+ case D_ABBREV:\n -+ strvec_pushf(&cmd.args, \"--abbrev=%d\",\n -+ atom->u.describe.length);\n -+ break;\n -+ case D_EXCLUDE:\n -+ strvec_pushf(&cmd.args, \"--exclude=%s\",\n -+ atom->u.describe.pattern);\n -+ break;\n -+ case D_MATCH:\n -+ strvec_pushf(&cmd.args, \"--match=%s\",\n -+ atom->u.describe.pattern);\n -+ break;\n -+ }\n -+\n ++ strvec_pushv(&cmd.args, atom->u.describe.args);\n + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n + error(_(\"failed to run 'describe'\"));\n2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a\ndifferent repo\n fivlite BR describe4 ~ | Documents | git git checkout master\nSwitched to branch 'master'\nYour branch is up to date with 'upstream/master'.\n fivlite BR master ~ | Documents | git git range-diff\n9748a6820043..describe4 master..describe5\n-: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing\nfunctions\n1: 9e3e652659 ! 2: f6f882884c ref-filter: add new \"describe\" atom\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n commits ahead and behind, respectively, when comparing the\noutput\n ref to the `` specified in the format.\n \n -+describe[:options]:: human-readable name, like\n ++describe[:options]:: Human-readable name, like\n + link-git:git-describe[1]; empty string for\n + undescribable commits. The `describe` string may be\n + followed by a colon and zero or more\ncomma-separated\n + options. Descriptions can be inconsistent when tags\n + are added or removed at the same time.\n ++\n -+** tags=: Instead of only considering annotated tags,\nconsider\n -+ lightweight tags as well.\n -+** abbrev=: Instead of using the default number of\nhexadecimal digits\n -+ (which will vary according to the number of objects\nin the\n -+ repository with a default of 7) of the abbreviated\n -+ object name, use digits, or as many digits\nas\n -+ needed to form a unique object name.\n -+** match=: Only consider tags matching the given\n`glob(7)` pattern,\n -+ excluding the \"refs/tags/\" prefix.\n -+** exclude=: Do not consider tags matching the given\n`glob(7)`\n -+ pattern,excluding the \"refs/tags/\" prefix.\n ++--\n ++tags=;; Instead of only considering annotated tags,\nconsider\n ++ lightweight tags as well; see the corresponding\noption\n ++ in linkgit:git-describe[1] for details.\n ++abbrev=;; Use at least hexadecimal digits; see\n ++ the corresponding option in linkgit:git-describe[1]\n ++ for details.\n ++match=;; Only consider tags matching the given `glob(7)`\npattern,\n ++ excluding the \"refs/tags/\" prefix; see the\ncorresponding\n...skipping...\n -+ case D_BARE:\n -+ break;\n -+ case D_TAGS:\n -+ if (atom->u.describe.tagbool)\n -+ strvec_push(&cmd.args, \"--tags\");\n -+ else\n -+ strvec_push(&cmd.args, \"--no-tags\");\n -+ break;\n -+ case D_ABBREV:\n -+ strvec_pushf(&cmd.args, \"--abbrev=%d\",\n -+ atom->u.describe.length);\n -+ break;\n -+ case D_EXCLUDE:\n -+ strvec_pushf(&cmd.args, \"--exclude=%s\",\n -+ atom->u.describe.pattern);\n -+ break;\n -+ case D_MATCH:\n -+ strvec_pushf(&cmd.args, \"--match=%s\",\n -+ atom->u.describe.pattern);\n -+ break;\n -+ }\n -+\n ++ strvec_pushv(&cmd.args, atom->u.describe.args);\n + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n + error(_(\"failed to run 'describe'\"));\n2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a\ndifferent repo\n\n-- \n2.41.0.321.g26b82700c0.dirty\n\n"},{"id":"479530","messageId":"20230714194249.66862-2-five231003@gmail.com","threadId":"59952","inReplyTo":"20230714194249.66862-1-five231003@gmail.com","subject":"[RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-14T19:20:26Z","receivedAt":"2023-07-14T19:43:21Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The functions\n\n\tmatch_placeholder_arg_value()\n\tmatch_placeholder_bool_arg()\n\nwere added in pretty (4f732e0fd7 (pretty: allow %(trailers) options\nwith explicit value, 2019-01-29)) to parse multiple options in an\nargument to --pretty. For example,\n\n\tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n\nwill output all the trailers matching the key and seperates them by\ncommas per commit.\n\nAdd similar functions,\n\n\tmatch_atom_arg_value()\n\tmatch_atom_bool_arg()\n\nin ref-filter. A particular use of this can be seen in the subsequent\ncommit where we parse the options given to a new atom \"describe\".\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 59 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e0d03a9f8e..b170994d9d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -251,6 +251,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)\n \treturn -1;\n }\n \n+static int match_atom_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, const char **valuestart,\n+\t\t\t\tsize_t *valuelen)\n+{\n+\tconst char *atom;\n+\n+\tif (!(skip_prefix(to_parse, candidate, &atom)))\n+\t\treturn 0;\n+\tif (valuestart) {\n+\t\tif (*atom == '=') {\n+\t\t\t*valuestart = atom + 1;\n+\t\t\t*valuelen = strcspn(*valuestart, \",\\0\");\n+\t\t\tatom = *valuestart + *valuelen;\n+\t\t} else {\n+\t\t\tif (*atom != ',' && *atom != '\\0')\n+\t\t\t\treturn 0;\n+\t\t\t*valuestart = NULL;\n+\t\t\t*valuelen = 0;\n+\t\t}\n+\t}\n+\tif (*atom == ',') {\n+\t\t*end = atom + 1;\n+\t\treturn 1;\n+\t}\n+\tif (*atom == '\\0') {\n+\t\t*end = atom;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, int *val)\n+{\n+\tconst char *argval;\n+\tchar *strval;\n+\tsize_t arglen;\n+\tint v;\n+\n+\tif (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))\n+\t\treturn 0;\n+\n+\tif (!argval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrval = xstrndup(argval, arglen);\n+\tv = git_parse_maybe_bool(strval);\n+\tfree(strval);\n+\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static int color_atom_parser(struct ref_format *format, struct used_atom *atom,\n \t\t\t const char *color_value, struct strbuf *err)\n {\n-- \n2.41.0.321.g26b82700c0.dirty\n\n"},{"id":"479532","messageId":"20230714194249.66862-3-five231003@gmail.com","threadId":"59952","inReplyTo":"20230714194249.66862-1-five231003@gmail.com","subject":"[PATCH v2 2/3] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-14T19:20:27Z","receivedAt":"2023-07-14T19:43:29Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Duplicate the logic of %(describe) and friends from pretty to\nref-filter. In the future, this change helps in unifying both the\nformats as ref-filter will be able to do everything that pretty is doing\nand we can have a single interface.\n\nThe new atom \"describe\" and its friends are equivalent to the existing\npretty formats with the same name.\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n Documentation/git-for-each-ref.txt | 23 +++++\n ref-filter.c | 147 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 85 +++++++++++++++++\n 3 files changed, 255 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 1e215d4e73..2a44119f38 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -231,6 +231,29 @@ ahead-behind:::\n \tcommits ahead and behind, respectively, when comparing the output\n \tref to the `` specified in the format.\n \n+describe[:options]:: Human-readable name, like\n+\t\t link-git:git-describe[1]; empty string for\n+\t\t undescribable commits. The `describe` string may be\n+\t\t followed by a colon and zero or more comma-separated\n+\t\t options. Descriptions can be inconsistent when tags\n+\t\t are added or removed at the same time.\n++\n+--\n+tags=;; Instead of only considering annotated tags, consider\n+\t\t lightweight tags as well; see the corresponding option\n+\t\t in linkgit:git-describe[1] for details.\n+abbrev=;; Use at least hexadecimal digits; see\n+\t\t the corresponding option in linkgit:git-describe[1]\n+\t\t for details.\n+match=;; Only consider tags matching the given `glob(7)` pattern,\n+\t\t excluding the \"refs/tags/\" prefix; see the corresponding\n+\t\t option in linkgit:git-describe[1] for details.\n+exclude=;; Do not consider tags matching the given `glob(7)`\n+\t\t pattern, excluding the \"refs/tags/\" prefix; see the\n+\t\t corresponding option in linkgit:git-describe[1] for\n+\t\t details.\n+--\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b170994d9d..fe4830dbea 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2,9 +2,11 @@\n #include \"alloc.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n+#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"object-name.h\"\n@@ -146,6 +148,7 @@ enum atom_type {\n \tATOM_TAGGERDATE,\n \tATOM_CREATOR,\n \tATOM_CREATORDATE,\n+\tATOM_DESCRIBE,\n \tATOM_SUBJECT,\n \tATOM_BODY,\n \tATOM_TRAILERS,\n@@ -215,6 +218,11 @@ static struct used_atom {\n \t\tstruct email_option {\n \t\t\tenum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n \t\t} email_option;\n+\t\tstruct {\n+\t\t\tenum { D_BARE, D_TAGS, D_ABBREV,\n+\t\t\t D_EXCLUDE, D_MATCH } option;\n+\t\t\tconst char **args;\n+\t\t} describe;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -521,6 +529,94 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \treturn 0;\n }\n \n+static int describe_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\tstruct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n+{\n+\tconst char *describe_opts[] = {\n+\t\t\"\",\n+\t\t\"tags\",\n+\t\t\"abbrev\",\n+\t\t\"match\",\n+\t\t\"exclude\",\n+\t\tNULL\n+\t};\n+\n+\tstruct strvec args = STRVEC_INIT;\n+\tfor (;;) {\n+\t\tint found = 0;\n+\t\tconst char *argval;\n+\t\tsize_t arglen = 0;\n+\t\tint optval = 0;\n+\t\tint opt;\n+\n+\t\tif (!arg)\n+\t\t\tbreak;\n+\n+\t\tfor (opt = D_BARE; !found && describe_opts[opt]; opt++) {\n+\t\t\tswitch(opt) {\n+\t\t\tcase D_BARE:\n+\t\t\t\t/*\n+\t\t\t\t * Do nothing. This is the bare describe\n+\t\t\t\t * atom and we already handle this above.\n+\t\t\t\t */\n+\t\t\t\tbreak;\n+\t\t\tcase D_TAGS:\n+\t\t\t\tif (match_atom_bool_arg(arg, describe_opts[opt],\n+\t\t\t\t\t\t\t&arg, &optval)) {\n+\t\t\t\t\tif (!optval)\n+\t\t\t\t\t\tstrvec_pushf(&args, \"--no-%s\",\n+\t\t\t\t\t\t\t describe_opts[opt]);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrvec_pushf(&args, \"--%s\",\n+\t\t\t\t\t\t\t describe_opts[opt]);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\tcase D_ABBREV:\n+\t\t\t\tif (match_atom_arg_value(arg, describe_opts[opt],\n+\t\t\t\t\t\t\t &arg, &argval, &arglen)) {\n+\t\t\t\t\tchar *endptr;\n+\t\t\t\t\tint ret = 0;\n+\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\tret = -1;\n+\t\t\t\t\tif (strtol(argval, &endptr, 10) < 0)\n+\t\t\t\t\t\tret = -1;\n+\t\t\t\t\tif (endptr - argval != arglen)\n+\t\t\t\t\t\tret = -1;\n+\n+\t\t\t\t\tif (ret)\n+\t\t\t\t\t\treturn strbuf_addf_ret(err, ret,\n+\t\t\t\t\t\t\t\t_(\"positive value expected describe:abbrev=%s\"), argval);\n+\t\t\t\t\tstrvec_pushf(&args, \"--%s=%.*s\",\n+\t\t\t\t\t\t describe_opts[opt],\n+\t\t\t\t\t\t (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\tcase D_MATCH:\n+\t\t\tcase D_EXCLUDE:\n+\t\t\t\tif (match_atom_arg_value(arg, describe_opts[opt],\n+\t\t\t\t\t\t\t &arg, &argval, &arglen)) {\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t\t\t\t_(\"value expected describe:%s=\"), describe_opts[opt]);\n+\t\t\t\t\tstrvec_pushf(&args, \"--%s=%.*s\",\n+\t\t\t\t\t\t describe_opts[opt],\n+\t\t\t\t\t\t (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (!found)\n+\t\t\tbreak;\n+\t}\n+\tatom->u.describe.args = strvec_detach(&args);\n+\treturn 0;\n+}\n+\n static int raw_atom_parser(struct ref_format *format UNUSED,\n \t\t\t struct used_atom *atom,\n \t\t\t const char *arg, struct strbuf *err)\n@@ -723,6 +819,7 @@ static struct {\n \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n+\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n@@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n \t}\n }\n \n+static void grab_describe_values(struct atom_value *val, int deref,\n+\t\t\t\t struct object *obj)\n+{\n+\tstruct commit *commit = (struct commit *)obj;\n+\tint i;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tenum atom_type type = atom->atom_type;\n+\t\tconst char *name = atom->name;\n+\t\tstruct atom_value *v = &val[i];\n+\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tif (type != ATOM_DESCRIBE)\n+\t\t\tcontinue;\n+\n+\t\tif (!!deref != (*name == '*'))\n+\t\t\tcontinue;\n+\t\tif (deref)\n+\t\t\tname++;\n+\n+\t\tif (!skip_prefix(name, \"describe\", &name) ||\n+\t\t (*name && *name != ':'))\n+\t\t\t continue;\n+\t\tif (!*name)\n+\t\t\tname = NULL;\n+\t\telse\n+\t\t\tname++;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_push(&cmd.args, \"describe\");\n+\t\tstrvec_pushv(&cmd.args, atom->u.describe.args);\n+\t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n+\t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n+\t\t\terror(_(\"failed to run 'describe'\"));\n+\t\t\tv->s = xstrdup(\"\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_rtrim(&out);\n+\t\tv->s = strbuf_detach(&out, NULL);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)\n {\n@@ -1651,12 +1796,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_tag_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 5c00607608..98ea37d336 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n+test_expect_success 'describe atom vs git describe' '\n+\ttest_when_finished \"rm -rf describe-repo\" &&\n+\n+\tgit init describe-repo &&\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\ttest_commit --no-tag one &&\n+\t\tgit tag tagone &&\n+\n+\t\ttest_commit --no-tag two &&\n+\t\tgit tag -a -m \"tag two\" tagtwo &&\n+\n+\t\tgit for-each-ref refs/tags/ --format=\"%(objectname)\" >obj &&\n+\t\twhile read hash\n+\t\tdo\n+\t\t\tif desc=$(git describe $hash)\n+\t\t\tthen\n+\t\t\t\t: >expect-contains-good\n+\t\t\telse\n+\t\t\t\t: >expect-contains-bad\n+\t\t\tfi &&\n+\t\t\techo \"$hash $desc\" || return 1\n+\t\tdone expect &&\n+\t\ttest_path_exists expect-contains-good &&\n+\t\ttest_path_exists expect-contains-bad &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname) %(describe)\" \\\n+\t\t\trefs/tags/ >actual 2>err &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'describe:tags vs describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit for-each-ref --format=\"%(describe:tags)\" refs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\n+\t# Case 1: We have commits between HEAD and the most\n+\t# recent tag reachable from it\n+\ttest_commit --no-tag file &&\n+\tgit describe --abbrev=14 >expect &&\n+\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Make sure the hash used is atleast 14 digits long\n+\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\ttest 15 -le $(wc -c expect &&\n+\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual &&\n+\ttest tagname = $(cat actual)\n+'\n+\n+test_expect_success 'describe:match=... vs describe --match ...' '\n+\ttest_when_finished \"git tag -d tag-match\" &&\n+\tgit tag -a -m \"tag match\" tag-match &&\n+\tgit describe --match \"*-match\" >expect &&\n+\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n+\ttest_when_finished \"git tag -d tag-exclude\" &&\n+\tgit tag -a -m \"tag exclude\" tag-exclude &&\n+\tgit describe --exclude \"*-exclude\" >expect &&\n+\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n+\t\trefs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat >expected <<\\EOF\n heads/main\n tags/main\n-- \n2.41.0.321.g26b82700c0.dirty\n\n"},{"id":"479531","messageId":"20230714194249.66862-4-five231003@gmail.com","threadId":"59952","inReplyTo":"20230714194249.66862-1-five231003@gmail.com","subject":"[PATCH v2 3/3] t6300: run describe atom tests on a different repo","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-14T19:20:28Z","receivedAt":"2023-07-14T19:43:30Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The tests for \"describe\" atom and its friends currently run on the main\nrepo of t6300, expect for the test for \"bare describe\", which is run on\n\"describe-repo\".\n\nThings can get messy with the other tests when such changes to a repo\nare done (for example, a new commit or a tag is introduced), especially\nin t6300 where the tests depend on commits and tags.\n\nAn example for this can be seen in [1], where writing the tests the\ncurrent way fails the test \"Verify sorts with raw:size\" on linux-sha256.\nThis, at first glance, seems totally unrelated.\n\nDigging in a bit deeper, it is apparent that this behavior is because of\nthe changes in the repo introduced when writing the \"describe\" tests,\nwhich changes the raw:size of an object. Such a change in raw-size would\nhave been, however, small if we were dealing with SHA1, but since we are\ndealing with SHA256, the change in raw:size is so significant that it\nfails the above mentioned test.\n\nSo, run all the \"describe\" atom tests on \"describe-repo\", which doesn't\ninterfere with the main repo on which the tests in t6300 are run.\n\n[1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++-----------------\n 1 file changed, 57 insertions(+), 44 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 98ea37d336..181b04e699 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n-test_expect_success 'describe atom vs git describe' '\n-\ttest_when_finished \"rm -rf describe-repo\" &&\n-\n+test_expect_success 'setup for describe atom tests' '\n \tgit init describe-repo &&\n \t(\n \t\tcd describe-repo &&\n@@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' '\n \t\tgit tag tagone &&\n \n \t\ttest_commit --no-tag two &&\n-\t\tgit tag -a -m \"tag two\" tagtwo &&\n+\t\tgit tag -a -m \"tag two\" tagtwo\n+\t)\n+'\n+\n+test_expect_success 'describe atom vs git describe' '\n+\t(\n+\t\tcd describe-repo &&\n \n-\t\tgit for-each-ref refs/tags/ --format=\"%(objectname)\" >obj &&\n+\t\tgit for-each-ref --format=\"%(objectname)\" \\\n+\t\t\trefs/tags/ >obj &&\n \t\twhile read hash\n \t\tdo\n \t\t\tif desc=$(git describe $hash)\n@@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' '\n '\n \n test_expect_success 'describe:tags vs describe --tags' '\n-\ttest_when_finished \"git tag -d tagname\" &&\n-\tgit tag tagname &&\n-\tgit describe --tags >expect &&\n-\tgit for-each-ref --format=\"%(describe:tags)\" refs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit describe --tags >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:tags)\" \\\n+\t\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n-\ttest_when_finished \"git tag -d tagname\" &&\n-\n-\t# Case 1: We have commits between HEAD and the most\n-\t# recent tag reachable from it\n-\ttest_commit --no-tag file &&\n-\tgit describe --abbrev=14 >expect &&\n-\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual &&\n-\n-\t# Make sure the hash used is atleast 14 digits long\n-\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n-\ttest 15 -le $(wc -c expect &&\n-\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual &&\n-\ttest tagname = $(cat actual)\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# Case 1: We have commits between HEAD and the most\n+\t\t#\t recent tag reachable from it\n+\t\ttest_commit --no-tag file &&\n+\t\tgit describe --abbrev=14 >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Make sure the hash used is atleast 14 digits long\n+\t\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\t\ttest 15 -le $(wc -c expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest tagname = $(cat actual)\n+\t)\n '\n \n test_expect_success 'describe:match=... vs describe --match ...' '\n-\ttest_when_finished \"git tag -d tag-match\" &&\n-\tgit tag -a -m \"tag match\" tag-match &&\n-\tgit describe --match \"*-match\" >expect &&\n-\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag match\" tag-match &&\n+\t\tgit describe --match \"*-match\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n-\ttest_when_finished \"git tag -d tag-exclude\" &&\n-\tgit tag -a -m \"tag exclude\" tag-exclude &&\n-\tgit describe --exclude \"*-exclude\" >expect &&\n-\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n-\t\trefs/heads/ >actual &&\n-\ttest_cmp expect actual\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag exclude\" tag-exclude &&\n+\t\tgit describe --exclude \"*-exclude\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n+\t\t\trefs/heads/ >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n '\n \n cat >expected <<\\EOF\n-- \n2.41.0.321.g26b82700c0.dirty\n\n"},{"id":"479534","messageId":"xmqqilamnrcr.fsf@gitster.g","threadId":"59952","inReplyTo":"20230714194249.66862-3-five231003@gmail.com","subject":"Re: [PATCH v2 2/3] ref-filter: add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-14T20:57:40Z","receivedAt":"2023-07-14T20:57:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> +\t\tstruct {\n> +\t\t\tenum { D_BARE, D_TAGS, D_ABBREV,\n> +\t\t\t D_EXCLUDE, D_MATCH } option;\n> +\t\t\tconst char **args;\n> +\t\t} describe;\n\nAs you parse this into a strvec that has command line options for\nthe \"git describe\" invocation, I do not see the point of having the\n\"enum option\" in this struct. The describe->option member seems to\nbe unused throughout this patch.\n\nIn fact, a single \"const char **describe_args\" should be able to\nreplace the structure, no?\n\n> +static int describe_atom_parser(struct ref_format *format UNUSED,\n> +\t\t\t\tstruct used_atom *atom,\n> +\t\t\t\tconst char *arg, struct strbuf *err)\n> +{\n> +\tconst char *describe_opts[] = {\n> +\t\t\"\",\n> +\t\t\"tags\",\n> +\t\t\"abbrev\",\n> +\t\t\"match\",\n> +\t\t\"exclude\",\n> +\t\tNULL\n> +\t};\n> +\n> +\tstruct strvec args = STRVEC_INIT;\n> +\tfor (;;) {\n> +\t\tint found = 0;\n> +\t\tconst char *argval;\n> +\t\tsize_t arglen = 0;\n> +\t\tint optval = 0;\n> +\t\tint opt;\n> +\n> +\t\tif (!arg)\n> +\t\t\tbreak;\n> +\n> +\t\tfor (opt = D_BARE; !found && describe_opts[opt]; opt++) {\n> +\t\t\tswitch(opt) {\n> +\t\t\tcase D_BARE:\n> +\t\t\t\t/*\n> +\t\t\t\t * Do nothing. This is the bare describe\n> +\t\t\t\t * atom and we already handle this above.\n> +\t\t\t\t */\n> +\t\t\t\tbreak;\n> +\t\t\tcase D_TAGS:\n> +\t\t\t\tif (match_atom_bool_arg(arg, describe_opts[opt],\n> +\t\t\t\t\t\t\t&arg, &optval)) {\n> +\t\t\t\t\tif (!optval)\n> +\t\t\t\t\t\tstrvec_pushf(&args, \"--no-%s\",\n> +\t\t\t\t\t\t\t describe_opts[opt]);\n> +\t\t\t\t\telse\n> +\t\t\t\t\t\tstrvec_pushf(&args, \"--%s\",\n> +\t\t\t\t\t\t\t describe_opts[opt]);\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t}\n\nAs match_atom_bool_arg() and ...\n\n> +\t\t\t\tbreak;\n> +\t\t\tcase D_ABBREV:\n> +\t\t\t\tif (match_atom_arg_value(arg, describe_opts[opt],\n> +\t\t\t\t\t\t\t &arg, &argval, &arglen)) {\n> +\t\t\t\t\tchar *endptr;\n> +\t\t\t\t\tint ret = 0;\n> +\n> +\t\t\t\t\tif (!arglen)\n> +\t\t\t\t\t\tret = -1;\n> +\t\t\t\t\tif (strtol(argval, &endptr, 10) < 0)\n> +\t\t\t\t\t\tret = -1;\n> +\t\t\t\t\tif (endptr - argval != arglen)\n> +\t\t\t\t\t\tret = -1;\n> +\n> +\t\t\t\t\tif (ret)\n> +\t\t\t\t\t\treturn strbuf_addf_ret(err, ret,\n> +\t\t\t\t\t\t\t\t_(\"positive value expected describe:abbrev=%s\"), argval);\n> +\t\t\t\t\tstrvec_pushf(&args, \"--%s=%.*s\",\n> +\t\t\t\t\t\t describe_opts[opt],\n> +\t\t\t\t\t\t (int)arglen, argval);\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t}\n\n... match_atom_arg_value() are both silent when they return false,\nwe do not see any diagnosis when these two case arms set the \"found\"\nflag. Shouldn't we have a corresponding \"else\" clause to these \"if\n(match_atom_blah())\" blocks to issue an error message or something?\n\n> +\t\t\t\tbreak;\n> +\t\t\tcase D_MATCH:\n> +\t\t\tcase D_EXCLUDE:\n> +\t\t\t\tif (match_atom_arg_value(arg, describe_opts[opt],\n> +\t\t\t\t\t\t\t &arg, &argval, &arglen)) {\n> +\t\t\t\t\tif (!arglen)\n> +\t\t\t\t\t\treturn strbuf_addf_ret(err, -1,\n> +\t\t\t\t\t\t\t\t_(\"value expected describe:%s=\"), describe_opts[opt]);\n> +\t\t\t\t\tstrvec_pushf(&args, \"--%s=%.*s\",\n> +\t\t\t\t\t\t describe_opts[opt],\n> +\t\t\t\t\t\t (int)arglen, argval);\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (!found)\n> +\t\t\tbreak;\n> +\t}\n> +\tatom->u.describe.args = strvec_detach(&args);\n> +\treturn 0;\n> +}\n> +\n> static int raw_atom_parser(struct ref_format *format UNUSED,\n> \t\t\t struct used_atom *atom,\n> \t\t\t const char *arg, struct strbuf *err)\n> @@ -723,6 +819,7 @@ static struct {\n> \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n> \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n> \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n> +\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n> \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n> \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n> \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n> @@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n> \t}\n> }\n> \n> +static void grab_describe_values(struct atom_value *val, int deref,\n> +\t\t\t\t struct object *obj)\n> +{\n> +\tstruct commit *commit = (struct commit *)obj;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < used_atom_cnt; i++) {\n> +\t\tstruct used_atom *atom = &used_atom[i];\n> +\t\tenum atom_type type = atom->atom_type;\n> +\t\tconst char *name = atom->name;\n> +\t\tstruct atom_value *v = &val[i];\n> +\n> +\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\t\tif (type != ATOM_DESCRIBE)\n> +\t\t\tcontinue;\n\nWe already have parsed the %(describe:...) and the result is stored\nin the used_atom[] array. We iterate over the array, and we just\nfound that its atom_type member is ATOM_DESCRIBE here (otherwise we\nwould have moved on to the next array element).\n\n> +\t\tif (!!deref != (*name == '*'))\n> +\t\t\tcontinue;\n\nThis is trying to avoid %(*describe) answering when the given object\nis the tag itself, or %(describe) answering when the given object is\nwhat the tag dereferences to, so having it here makes sense (by the\nway, do you add any test for \"%(*describe)?\").\n\nNow, is the code from here ...\n\n> +\t\tif (deref)\n> +\t\t\tname++;\n> +\n> +\t\tif (!skip_prefix(name, \"describe\", &name) ||\n> +\t\t (*name && *name != ':'))\n> +\t\t\t continue;\n> +\t\tif (!*name)\n> +\t\t\tname = NULL;\n> +\t\telse\n> +\t\t\tname++;\n\n... down to here doing anything useful? After all, you already have\nall you need to describe the commit in atom->u.describe_args to run\n\"git describe\" with, no? In fact, after computing \"name\" with the\nabove code with some complexity, nobody even looks at it.\n\nPerhaps the above was copied from some other grab_* functions; the\nreason why they were relevant there needs to be understood, and it\nalso has to be considered if the same reason to have the code here\napplies to this codepath.\n\n"},{"id":"479546","messageId":"ZLLkZ4Vx2quwWwRz@five231003","threadId":"59952","inReplyTo":"xmqqilamnrcr.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-15T18:24:39Z","receivedAt":"2023-07-15T18:24:56Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Fri, Jul 14, 2023 at 01:57:40PM -0700, Junio C Hamano wrote:\n> Kousik Sanagavarapu writes:\n> \n> > +\t\tstruct {\n> > +\t\t\tenum { D_BARE, D_TAGS, D_ABBREV,\n> > +\t\t\t D_EXCLUDE, D_MATCH } option;\n> > +\t\t\tconst char **args;\n> > +\t\t} describe;\n> \n> As you parse this into a strvec that has command line options for\n> the \"git describe\" invocation, I do not see the point of having the\n> \"enum option\" in this struct. The describe->option member seems to\n> be unused throughout this patch.\n> \n> In fact, a single \"const char **describe_args\" should be able to\n> replace the structure, no?\n\nI kept the enum because I thought it could act as an index for the\ndescribe_opts array. Now that I think about it,\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fe4830dbea..df7cb39be2 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -219,9 +219,7 @@ static struct used_atom {\n \t\t\tenum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n \t\t} email_option;\n \t\tstruct {\n-\t\t\tenum { D_BARE, D_TAGS, D_ABBREV,\n-\t\t\t D_EXCLUDE, D_MATCH } option;\n-\t\t\tconst char **args;\n+\t\t\tconst char **decsribe_args;\n \t\t} describe;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n@@ -533,13 +531,16 @@ static int describe_atom_parser(struct ref_format *format UNUSED,\n \t\t\t\tstruct used_atom *atom,\n \t\t\t\tconst char *arg, struct strbuf *err)\n {\n-\tconst char *describe_opts[] = {\n-\t\t\"\",\n-\t\t\"tags\",\n-\t\t\"abbrev\",\n-\t\t\"match\",\n-\t\t\"exclude\",\n-\t\tNULL\n+\tstruct {\n+\t\tchar *optname;\n+\t\tenum { D_BARE, D_TAGS, D_ABBREV,\n+\t\t D_MATCH, D_EXCLUDE } option;\n+\t} describe_opts[] = {\n+\t\t{ \"\", D_BARE },\n+\t\t{ \"tags\", D_TAGS },\n+\t\t{ \"abbrev\", D_ABBREV },\n+\t\t{ \"match\", D_MATCH },\n+\t\t{ \"exclude\", D_EXCLUDE }\n \t};\n \n \tstruct strvec args = STRVEC_INIT;\n\nconveys it better or is it too much unnecessary stuff to and should we\njust do\n\n\tstruct {\n\t\tconst char **describe_args;\n\t} describe;\n\nleaving the describe_opts array as is and changing the how the switch is\nwritten.\n\n> > +static int describe_atom_parser(struct ref_format *format UNUSED,\n> > +\t\t\t\tstruct used_atom *atom,\n> > +\t\t\t\tconst char *arg, struct strbuf *err)\n> > +{\n> > +\tconst char *describe_opts[] = {\n> > +\t\t\"\",\n> > +\t\t\"tags\",\n> > +\t\t\"abbrev\",\n> > +\t\t\"match\",\n> > +\t\t\"exclude\",\n> > +\t\tNULL\n> > +\t};\n> > +\n> > +\tstruct strvec args = STRVEC_INIT;\n> > +\tfor (;;) {\n> > +\t\tint found = 0;\n> > +\t\tconst char *argval;\n> > +\t\tsize_t arglen = 0;\n> > +\t\tint optval = 0;\n> > +\t\tint opt;\n> > +\n> > +\t\tif (!arg)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tfor (opt = D_BARE; !found && describe_opts[opt]; opt++) {\n> > +\t\t\tswitch(opt) {\n> > +\t\t\tcase D_BARE:\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Do nothing. This is the bare describe\n> > +\t\t\t\t * atom and we already handle this above.\n> > +\t\t\t\t */\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase D_TAGS:\n> > +\t\t\t\tif (match_atom_bool_arg(arg, describe_opts[opt],\n> > +\t\t\t\t\t\t\t&arg, &optval)) {\n> > +\t\t\t\t\tif (!optval)\n> > +\t\t\t\t\t\tstrvec_pushf(&args, \"--no-%s\",\n> > +\t\t\t\t\t\t\t describe_opts[opt]);\n> > +\t\t\t\t\telse\n> > +\t\t\t\t\t\tstrvec_pushf(&args, \"--%s\",\n> > +\t\t\t\t\t\t\t describe_opts[opt]);\n> > +\t\t\t\t\tfound = 1;\n> > +\t\t\t\t}\n> \n> As match_atom_bool_arg() and ...\n> \n> > +\t\t\t\tbreak;\n> > +\t\t\tcase D_ABBREV:\n> > +\t\t\t\tif (match_atom_arg_value(arg, describe_opts[opt],\n> > +\t\t\t\t\t\t\t &arg, &argval, &arglen)) {\n> > +\t\t\t\t\tchar *endptr;\n> > +\t\t\t\t\tint ret = 0;\n> > +\n> > +\t\t\t\t\tif (!arglen)\n> > +\t\t\t\t\t\tret = -1;\n> > +\t\t\t\t\tif (strtol(argval, &endptr, 10) < 0)\n> > +\t\t\t\t\t\tret = -1;\n> > +\t\t\t\t\tif (endptr - argval != arglen)\n> > +\t\t\t\t\t\tret = -1;\n> > +\n> > +\t\t\t\t\tif (ret)\n> > +\t\t\t\t\t\treturn strbuf_addf_ret(err, ret,\n> > +\t\t\t\t\t\t\t\t_(\"positive value expected describe:abbrev=%s\"), argval);\n> > +\t\t\t\t\tstrvec_pushf(&args, \"--%s=%.*s\",\n> > +\t\t\t\t\t\t describe_opts[opt],\n> > +\t\t\t\t\t\t (int)arglen, argval);\n> > +\t\t\t\t\tfound = 1;\n> > +\t\t\t\t}\n> \n> ... match_atom_arg_value() are both silent when they return false,\n> we do not see any diagnosis when these two case arms set the \"found\"\n> flag. Shouldn't we have a corresponding \"else\" clause to these \"if\n> (match_atom_blah())\" blocks to issue an error message or something?\n\nYeah, I'll add this.\n\n> [...] \n> Now, is the code from here ...\n> \n> > +\t\tif (deref)\n> > +\t\t\tname++;\n> > +\n> > +\t\tif (!skip_prefix(name, \"describe\", &name) ||\n> > +\t\t (*name && *name != ':'))\n> > +\t\t\t continue;\n> > +\t\tif (!*name)\n> > +\t\t\tname = NULL;\n> > +\t\telse\n> > +\t\t\tname++;\n> \n> ... down to here doing anything useful? After all, you already have\n> all you need to describe the commit in atom->u.describe_args to run\n> \"git describe\" with, no? In fact, after computing \"name\" with the\n> above code with some complexity, nobody even looks at it.\n> \n> Perhaps the above was copied from some other grab_* functions; the\n> reason why they were relevant there needs to be understood, and it\n> also has to be considered if the same reason to have the code here\n> applies to this codepath.\n\nSorry you had to read through this. I'll remove these if constructs,\nbecause as you said, they do nothing since we already parse everything\nwe need and also check for the type and the deref.\n\nThere is not test for \"%(*describe)\", but I'll add one in v3 if you\nthink it is necessary (if we are doing this, should we also do one for\nmultiple options?).\n\nThanks\n"},{"id":"479547","messageId":"xmqq351pm2ai.fsf@gitster.g","threadId":"59952","inReplyTo":"ZLLkZ4Vx2quwWwRz@five231003","subject":"Re: [PATCH v2 2/3] ref-filter: add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-15T18:56:37Z","receivedAt":"2023-07-15T19:00:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> conveys it better or is it too much unnecessary stuff to and should we\n> just do\n>\n> \tstruct {\n> \t\tconst char **describe_args;\n> \t} describe;\n>\n> leaving the describe_opts array as is and changing the how the switch is\n> written.\n\nI think this struct can be replaced with a single\n\n\tconst char **describe_args;\n\nand then\n\n>> > +static int describe_atom_parser(struct ref_format *format UNUSED,\n>> > +\t\t\t\tstruct used_atom *atom,\n>> > +\t\t\t\tconst char *arg, struct strbuf *err)\n>> > +{\n>> > +\tconst char *describe_opts[] = {\n>> > +\t\t\"\",\n>> > +\t\t\"tags\",\n>> > +\t\t\"abbrev\",\n>> > +\t\t\"match\",\n>> > +\t\t\"exclude\",\n>> > +\t\tNULL\n>> > +\t};\n\nthis array can simply go away. Then you can\n\n>> > +\tstruct strvec args = STRVEC_INIT;\n>> > +\tfor (;;) {\n>> > +\t\tint found = 0;\n>> > +\t\tconst char *argval;\n>> > +\t\tsize_t arglen = 0;\n>> > +\t\tint optval = 0;\n>> > +\t\tint opt;\n>> > +\n>> > +\t\tif (!arg)\n>> > +\t\t\tbreak;\n>> > +\n>> > +\t\tfor (opt = D_BARE; !found && describe_opts[opt]; opt++) {\n\nrewrite this \"for\" loop plus the \"switch\" inside to an if/else\nif/else cascade:\n\n\t\tif (match_atom_bool_arg(arg, \"tags\", &arg, &optval)) {\n\t\t\t... do \"tags\" thing ...\n\t\t} else if (match_atom_arg_value(arg, \"abbrev\", ...)) {\n\t\t\t... do \"abbrev\" thing ...\n\t\t} else if ...\n\nThat way, you do not need any enum anywhere and there is no reason\nto have desribe_opts[] array, either.\n\n\n"},{"id":"479640","messageId":"20230719162424.70781-1-five231003@gmail.com","threadId":"59952","inReplyTo":"20230714194249.66862-1-five231003@gmail.com","subject":"[PATCH v3 0/2] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-19T16:15:04Z","receivedAt":"2023-07-19T16:24:47Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\nThanks Junio for the review on the previous version of this series. I\nhave made the changes according to the comments left.\n\nPATCH 1/2 - Left unchanged expect for small changes in the commit\n\t message for more clarity.\n\nPATCH 2/2 - We now parse the arguments in a seperate function\n\t `describe_atom_option_parser()` call this in\n\t `describe_atom_parser()` instead to populate\n\t `atom->u.describe_args`. This splitting of the function\n\t helps err at the right places.\n\n\t I've also squashed the third commit in the previous version\n\t into this commit.\n\nKousik Sanagavarapu (2):\n ref-filter: add multiple-option parsing functions\n ref-filter: add new \"describe\" atom\n\n Documentation/git-for-each-ref.txt | 23 ++++\n ref-filter.c | 189 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 114 +++++++++++++++++\n 3 files changed, 326 insertions(+)\n\nRange-diff against v2:\n\n1: 50497067a3 ! 1: 08f3be1631 ref-filter: add multiple-option parsing functions\n @@ Commit message\n match_placeholder_arg_value()\n match_placeholder_bool_arg()\n \n - were added in pretty (4f732e0fd7 (pretty: allow %(trailers) options\n - with explicit value, 2019-01-29)) to parse multiple options in an\n + were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\n + with explicit value, 2019-01-29) to parse multiple options in an\n argument to --pretty. For example,\n \n git log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n \n will output all the trailers matching the key and seperates them by\n - commas per commit.\n + a comma followed by a space per commit.\n \n Add similar functions,\n \n match_atom_arg_value()\n match_atom_bool_arg()\n \n - in ref-filter. A particular use of this can be seen in the subsequent\n - commit where we parse the options given to a new atom \"describe\".\n + in ref-filter.\n +\n + There is no atom yet that can use these functions in ref-filter, but we\n + are going to add a new %(describe) atom in a subsequent commit where we\n + parse options like tags= or match= given to it.\n \n Mentored-by: Christian Couder \n Mentored-by: Hariom Verma \n2: f6f882884c ! 2: 742a79113c ref-filter: add new \"describe\" atom\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n \n ## ref-filter.c ##\n @@\n - #include \"alloc.h\"\n + #include \"git-compat-util.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n +#include \"config.h\"\n @@ ref-filter.c: enum atom_type {\n ATOM_BODY,\n ATOM_TRAILERS,\n @@ ref-filter.c: static struct used_atom {\n - struct email_option {\n - enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;\n - } email_option;\n -+ struct {\n -+ enum { D_BARE, D_TAGS, D_ABBREV,\n -+ D_EXCLUDE, D_MATCH } option;\n -+ const char **args;\n -+ } describe;\n + enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n + S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n + } signature;\n ++ const char **describe_args;\n struct refname_atom refname;\n char *head;\n } u;\n @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct\n return 0;\n }\n \n ++static int describe_atom_option_parser(struct strvec *args, const char **arg,\n ++ struct strbuf *err)\n ++{\n ++ const char *argval;\n ++ size_t arglen = 0;\n ++ int optval = 0;\n ++\n ++ if (match_atom_bool_arg(*arg, \"tags\", arg, &optval)) {\n ++ if (!optval)\n ++ strvec_push(args, \"--no-tags\");\n ++ else\n ++ strvec_push(args, \"--tags\");\n ++ return 1;\n ++ }\n ++\n ++ if (match_atom_arg_value(*arg, \"abbrev\", arg, &argval, &arglen)) {\n ++ char *endptr;\n ++\n ++ if (!arglen)\n ++ return strbuf_addf_ret(err, -1,\n ++ _(\"argument expected for %s\"),\n ++ \"describe:abbrev\");\n ++ if (strtol(argval, &endptr, 10) < 0)\n ++ return strbuf_addf_ret(err, -1,\n ++ _(\"positive value expected %s=%s\"),\n ++ \"describe:abbrev\", argval);\n ++ if (endptr - argval != arglen)\n ++ return strbuf_addf_ret(err, -1,\n ++ _(\"cannot fully parse %s=%s\"),\n ++ \"describe:abbrev\", argval);\n ++\n ++ strvec_pushf(args, \"--abbrev=%.*s\", (int)arglen, argval);\n ++ return 1;\n ++ }\n ++\n ++ if (match_atom_arg_value(*arg, \"match\", arg, &argval, &arglen)) {\n ++ if (!arglen)\n ++ return strbuf_addf_ret(err, -1,\n ++ _(\"value expected %s=\"),\n ++ \"describe:match\");\n ++\n ++ strvec_pushf(args, \"--match=%.*s\", (int)arglen, argval);\n ++ return 1;\n ++ }\n ++\n ++ if (match_atom_arg_value(*arg, \"exclude\", arg, &argval, &arglen)) {\n ++ if (!arglen)\n ++ return strbuf_addf_ret(err, -1,\n ++ _(\"value expected %s=\"),\n ++ \"describe:exclude\");\n ++\n ++ strvec_pushf(args, \"--exclude=%.*s\", (int)arglen, argval);\n ++ return 1;\n ++ }\n ++\n ++ return 0;\n ++}\n ++\n +static int describe_atom_parser(struct ref_format *format UNUSED,\n + struct used_atom *atom,\n + const char *arg, struct strbuf *err)\n +{\n -+ const char *describe_opts[] = {\n -+ \"\",\n -+ \"tags\",\n -+ \"abbrev\",\n -+ \"match\",\n -+ \"exclude\",\n -+ NULL\n -+ };\n -+\n + struct strvec args = STRVEC_INIT;\n ++\n + for (;;) {\n + int found = 0;\n -+ const char *argval;\n -+ size_t arglen = 0;\n -+ int optval = 0;\n -+ int opt;\n ++ const char *bad_arg = NULL;\n +\n -+ if (!arg)\n ++ if (!arg || !*arg)\n + break;\n +\n -+ for (opt = D_BARE; !found && describe_opts[opt]; opt++) {\n -+ switch(opt) {\n -+ case D_BARE:\n -+ /*\n -+ * Do nothing. This is the bare describe\n -+ * atom and we already handle this above.\n -+ */\n -+ break;\n -+ case D_TAGS:\n -+ if (match_atom_bool_arg(arg, describe_opts[opt],\n -+ &arg, &optval)) {\n -+ if (!optval)\n -+ strvec_pushf(&args, \"--no-%s\",\n -+ describe_opts[opt]);\n -+ else\n -+ strvec_pushf(&args, \"--%s\",\n -+ describe_opts[opt]);\n -+ found = 1;\n -+ }\n -+ break;\n -+ case D_ABBREV:\n -+ if (match_atom_arg_value(arg, describe_opts[opt],\n -+ &arg, &argval, &arglen)) {\n -+ char *endptr;\n -+ int ret = 0;\n -+\n -+ if (!arglen)\n -+ ret = -1;\n -+ if (strtol(argval, &endptr, 10) < 0)\n -+ ret = -1;\n -+ if (endptr - argval != arglen)\n -+ ret = -1;\n -+\n -+ if (ret)\n -+ return strbuf_addf_ret(err, ret,\n -+ _(\"positive value expected describe:abbrev=%s\"), argval);\n -+ strvec_pushf(&args, \"--%s=%.*s\",\n -+ describe_opts[opt],\n -+ (int)arglen, argval);\n -+ found = 1;\n -+ }\n -+ break;\n -+ case D_MATCH:\n -+ case D_EXCLUDE:\n -+ if (match_atom_arg_value(arg, describe_opts[opt],\n -+ &arg, &argval, &arglen)) {\n -+ if (!arglen)\n -+ return strbuf_addf_ret(err, -1,\n -+ _(\"value expected describe:%s=\"), describe_opts[opt]);\n -+ strvec_pushf(&args, \"--%s=%.*s\",\n -+ describe_opts[opt],\n -+ (int)arglen, argval);\n -+ found = 1;\n -+ }\n -+ break;\n -+ }\n -+ }\n -+ if (!found)\n ++ bad_arg = arg;\n ++ found = describe_atom_option_parser(&args, &arg, err);\n ++ if (found < 0)\n ++ return found;\n ++ if (!found) {\n ++ if (bad_arg && *bad_arg)\n ++ return err_bad_arg(err, \"describe\", bad_arg);\n + break;\n ++ }\n + }\n -+ atom->u.describe.args = strvec_detach(&args);\n ++ atom->u.describe_args = strvec_detach(&args);\n + return 0;\n +}\n +\n @@ ref-filter.c: static void append_lines(struct strbuf *out, const char *buf, unsi\n +\n + if (!!deref != (*name == '*'))\n + continue;\n -+ if (deref)\n -+ name++;\n -+\n -+ if (!skip_prefix(name, \"describe\", &name) ||\n -+ (*name && *name != ':'))\n -+ continue;\n -+ if (!*name)\n -+ name = NULL;\n -+ else\n -+ name++;\n +\n + cmd.git_cmd = 1;\n + strvec_push(&cmd.args, \"describe\");\n -+ strvec_pushv(&cmd.args, atom->u.describe.args);\n ++ strvec_pushv(&cmd.args, atom->u.describe_args);\n + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n + error(_(\"failed to run 'describe'\"));\n @@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct\n break;\n case OBJ_COMMIT:\n grab_commit_values(val, deref, obj);\n - grab_sub_body_contents(val, deref, data);\n +@@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n grab_person(\"author\", val, deref, buf);\n grab_person(\"committer\", val, deref, buf);\n + grab_signature(val, deref, obj);\n + grab_describe_values(val, deref, obj);\n break;\n case OBJ_TREE:\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n test_cmp expected.bare actual\n '\n \n -+test_expect_success 'describe atom vs git describe' '\n -+ test_when_finished \"rm -rf describe-repo\" &&\n -+\n ++test_expect_success 'setup for describe atom tests' '\n + git init describe-repo &&\n + (\n + cd describe-repo &&\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n + git tag tagone &&\n +\n + test_commit --no-tag two &&\n -+ git tag -a -m \"tag two\" tagtwo &&\n ++ git tag -a -m \"tag two\" tagtwo\n ++ )\n ++'\n +\n -+ git for-each-ref refs/tags/ --format=\"%(objectname)\" >obj &&\n ++test_expect_success 'describe atom vs git describe' '\n ++ (\n ++ cd describe-repo &&\n ++\n ++ git for-each-ref --format=\"%(objectname)\" \\\n ++ refs/tags/ >obj &&\n + while read hash\n + do\n + if desc=$(git describe $hash)\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n +'\n +\n +test_expect_success 'describe:tags vs describe --tags' '\n -+ test_when_finished \"git tag -d tagname\" &&\n -+ git tag tagname &&\n -+ git describe --tags >expect &&\n -+ git for-each-ref --format=\"%(describe:tags)\" refs/heads/ >actual &&\n -+ test_cmp expect actual\n ++ (\n ++ cd describe-repo &&\n ++ git describe --tags >expect &&\n ++ git for-each-ref --format=\"%(describe:tags)\" \\\n ++ refs/heads/master >actual &&\n ++ test_cmp expect actual\n ++ )\n +'\n +\n +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n -+ test_when_finished \"git tag -d tagname\" &&\n -+\n -+ # Case 1: We have commits between HEAD and the most\n -+ # recent tag reachable from it\n -+ test_commit --no-tag file &&\n -+ git describe --abbrev=14 >expect &&\n -+ git for-each-ref --format=\"%(describe:abbrev=14)\" \\\n -+ refs/heads/ >actual &&\n -+ test_cmp expect actual &&\n -+\n -+ # Make sure the hash used is atleast 14 digits long\n -+ sed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n -+ test 15 -le $(wc -c expect &&\n -+ git for-each-ref --format=\"%(describe:abbrev=14)\" \\\n -+ refs/heads/ >actual &&\n -+ test_cmp expect actual &&\n -+ test tagname = $(cat actual)\n ++ (\n ++ cd describe-repo &&\n ++\n ++ # Case 1: We have commits between HEAD and the most\n ++ # recent tag reachable from it\n ++ test_commit --no-tag file &&\n ++ git describe --abbrev=14 >expect &&\n ++ git for-each-ref --format=\"%(describe:abbrev=14)\" \\\n ++ refs/heads/master >actual &&\n ++ test_cmp expect actual &&\n ++\n ++ # Make sure the hash used is atleast 14 digits long\n ++ sed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n ++ test 15 -le $(wc -c expect &&\n ++ git for-each-ref --format=\"%(describe:abbrev=14)\" \\\n ++ refs/heads/master >actual &&\n ++ test_cmp expect actual &&\n ++ test tagname = $(cat actual)\n ++ )\n +'\n +\n +test_expect_success 'describe:match=... vs describe --match ...' '\n -+ test_when_finished \"git tag -d tag-match\" &&\n -+ git tag -a -m \"tag match\" tag-match &&\n -+ git describe --match \"*-match\" >expect &&\n -+ git for-each-ref --format=\"%(describe:match=\"*-match\")\" \\\n -+ refs/heads/ >actual &&\n -+ test_cmp expect actual\n ++ (\n ++ cd describe-repo &&\n ++ git tag -a -m \"tag foo\" tag-foo &&\n ++ git describe --match \"*-foo\" >expect &&\n ++ git for-each-ref --format=\"%(describe:match=\"*-foo\")\" \\\n ++ refs/heads/master >actual &&\n ++ test_cmp expect actual\n ++ )\n +'\n +\n +test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n -+ test_when_finished \"git tag -d tag-exclude\" &&\n -+ git tag -a -m \"tag exclude\" tag-exclude &&\n -+ git describe --exclude \"*-exclude\" >expect &&\n -+ git for-each-ref --format=\"%(describe:exclude=\"*-exclude\")\" \\\n -+ refs/heads/ >actual &&\n -+ test_cmp expect actual\n ++ (\n ++ cd describe-repo &&\n ++ git tag -a -m \"tag bar\" tag-bar &&\n ++ git describe --exclude \"*-bar\" >expect &&\n ++ git for-each-ref --format=\"%(describe:exclude=\"*-bar\")\" \\\n ++ refs/heads/master >actual &&\n ++ test_cmp expect actual\n ++ )\n ++'\n ++\n ++test_expect_success 'deref with describe atom' '\n ++ (\n ++ cd describe-repo &&\n ++ cat >expect <<-\\EOF &&\n ++\n ++ tagname\n ++ tagname\n ++ tagname\n ++\n ++ tagtwo\n ++ EOF\n ++ git for-each-ref --format=\"%(*describe)\" >actual &&\n ++ test_cmp expect actual\n ++ )\n +'\n +\n cat >expected <<\\EOF\n3: a5122bf5e2 < -: ---------- t6300: run describe atom tests on a different repo\n\n-- \n2.41.0.378.g42703afc1f.dirty\n\n"},{"id":"479641","messageId":"20230719162424.70781-2-five231003@gmail.com","threadId":"59952","inReplyTo":"20230719162424.70781-1-five231003@gmail.com","subject":"[PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-19T16:15:05Z","receivedAt":"2023-07-19T16:24:52Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The functions\n\n\tmatch_placeholder_arg_value()\n\tmatch_placeholder_bool_arg()\n\nwere added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\nwith explicit value, 2019-01-29) to parse multiple options in an\nargument to --pretty. For example,\n\n\tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n\nwill output all the trailers matching the key and seperates them by\na comma followed by a space per commit.\n\nAdd similar functions,\n\n\tmatch_atom_arg_value()\n\tmatch_atom_bool_arg()\n\nin ref-filter.\n\nThere is no atom yet that can use these functions in ref-filter, but we\nare going to add a new %(describe) atom in a subsequent commit where we\nparse options like tags= or match= given to it.\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 59 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 60919f375f..f64437e781 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -255,6 +255,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)\n \treturn -1;\n }\n \n+static int match_atom_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, const char **valuestart,\n+\t\t\t\tsize_t *valuelen)\n+{\n+\tconst char *atom;\n+\n+\tif (!(skip_prefix(to_parse, candidate, &atom)))\n+\t\treturn 0;\n+\tif (valuestart) {\n+\t\tif (*atom == '=') {\n+\t\t\t*valuestart = atom + 1;\n+\t\t\t*valuelen = strcspn(*valuestart, \",\\0\");\n+\t\t\tatom = *valuestart + *valuelen;\n+\t\t} else {\n+\t\t\tif (*atom != ',' && *atom != '\\0')\n+\t\t\t\treturn 0;\n+\t\t\t*valuestart = NULL;\n+\t\t\t*valuelen = 0;\n+\t\t}\n+\t}\n+\tif (*atom == ',') {\n+\t\t*end = atom + 1;\n+\t\treturn 1;\n+\t}\n+\tif (*atom == '\\0') {\n+\t\t*end = atom;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, int *val)\n+{\n+\tconst char *argval;\n+\tchar *strval;\n+\tsize_t arglen;\n+\tint v;\n+\n+\tif (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))\n+\t\treturn 0;\n+\n+\tif (!argval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrval = xstrndup(argval, arglen);\n+\tv = git_parse_maybe_bool(strval);\n+\tfree(strval);\n+\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static int color_atom_parser(struct ref_format *format, struct used_atom *atom,\n \t\t\t const char *color_value, struct strbuf *err)\n {\n-- \n2.41.0.378.g42703afc1f.dirty\n\n"},{"id":"479642","messageId":"20230719162424.70781-3-five231003@gmail.com","threadId":"59952","inReplyTo":"20230719162424.70781-1-five231003@gmail.com","subject":"[PATCH v3 2/2] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-19T16:15:06Z","receivedAt":"2023-07-19T16:24:58Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Duplicate the logic of %(describe) and friends from pretty to\nref-filter. In the future, this change helps in unifying both the\nformats as ref-filter will be able to do everything that pretty is doing\nand we can have a single interface.\n\nThe new atom \"describe\" and its friends are equivalent to the existing\npretty formats with the same name.\n\nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n Documentation/git-for-each-ref.txt | 23 +++++\n ref-filter.c | 130 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++\n 3 files changed, 267 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2e0318770b..395daf1b22 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -258,6 +258,29 @@ ahead-behind:::\n \tcommits ahead and behind, respectively, when comparing the output\n \tref to the `` specified in the format.\n \n+describe[:options]:: Human-readable name, like\n+\t\t link-git:git-describe[1]; empty string for\n+\t\t undescribable commits. The `describe` string may be\n+\t\t followed by a colon and zero or more comma-separated\n+\t\t options. Descriptions can be inconsistent when tags\n+\t\t are added or removed at the same time.\n++\n+--\n+tags=;; Instead of only considering annotated tags, consider\n+\t\t lightweight tags as well; see the corresponding option\n+\t\t in linkgit:git-describe[1] for details.\n+abbrev=;; Use at least hexadecimal digits; see\n+\t\t the corresponding option in linkgit:git-describe[1]\n+\t\t for details.\n+match=;; Only consider tags matching the given `glob(7)` pattern,\n+\t\t excluding the \"refs/tags/\" prefix; see the corresponding\n+\t\t option in linkgit:git-describe[1] for details.\n+exclude=;; Do not consider tags matching the given `glob(7)`\n+\t\t pattern, excluding the \"refs/tags/\" prefix; see the\n+\t\t corresponding option in linkgit:git-describe[1] for\n+\t\t details.\n+--\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f64437e781..43316643be 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1,9 +1,11 @@\n #include \"git-compat-util.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n+#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"object-name.h\"\n@@ -145,6 +147,7 @@ enum atom_type {\n \tATOM_TAGGERDATE,\n \tATOM_CREATOR,\n \tATOM_CREATORDATE,\n+\tATOM_DESCRIBE,\n \tATOM_SUBJECT,\n \tATOM_BODY,\n \tATOM_TRAILERS,\n@@ -219,6 +222,7 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tconst char **describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -554,6 +558,91 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \treturn 0;\n }\n \n+static int describe_atom_option_parser(struct strvec *args, const char **arg,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tconst char *argval;\n+\tsize_t arglen = 0;\n+\tint optval = 0;\n+\n+\tif (match_atom_bool_arg(*arg, \"tags\", arg, &optval)) {\n+\t\tif (!optval)\n+\t\t\tstrvec_push(args, \"--no-tags\");\n+\t\telse\n+\t\t\tstrvec_push(args, \"--tags\");\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"abbrev\", arg, &argval, &arglen)) {\n+\t\tchar *endptr;\n+\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"argument expected for %s\"),\n+\t\t\t\t\t \"describe:abbrev\");\n+\t\tif (strtol(argval, &endptr, 10) < 0)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"positive value expected %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\t\tif (endptr - argval != arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"cannot fully parse %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\n+\t\tstrvec_pushf(args, \"--abbrev=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"match\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:match\");\n+\n+\t\tstrvec_pushf(args, \"--match=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"exclude\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:exclude\");\n+\n+\t\tstrvec_pushf(args, \"--exclude=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int describe_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\tstruct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\n+\tfor (;;) {\n+\t\tint found = 0;\n+\t\tconst char *bad_arg = NULL;\n+\n+\t\tif (!arg || !*arg)\n+\t\t\tbreak;\n+\n+\t\tbad_arg = arg;\n+\t\tfound = describe_atom_option_parser(&args, &arg, err);\n+\t\tif (found < 0)\n+\t\t\treturn found;\n+\t\tif (!found) {\n+\t\t\tif (bad_arg && *bad_arg)\n+\t\t\t\treturn err_bad_arg(err, \"describe\", bad_arg);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tatom->u.describe_args = strvec_detach(&args);\n+\treturn 0;\n+}\n+\n static int raw_atom_parser(struct ref_format *format UNUSED,\n \t\t\t struct used_atom *atom,\n \t\t\t const char *arg, struct strbuf *err)\n@@ -756,6 +845,7 @@ static struct {\n \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n+\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n@@ -1662,6 +1752,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n \t}\n }\n \n+static void grab_describe_values(struct atom_value *val, int deref,\n+\t\t\t\t struct object *obj)\n+{\n+\tstruct commit *commit = (struct commit *)obj;\n+\tint i;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tenum atom_type type = atom->atom_type;\n+\t\tconst char *name = atom->name;\n+\t\tstruct atom_value *v = &val[i];\n+\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tif (type != ATOM_DESCRIBE)\n+\t\t\tcontinue;\n+\n+\t\tif (!!deref != (*name == '*'))\n+\t\t\tcontinue;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_push(&cmd.args, \"describe\");\n+\t\tstrvec_pushv(&cmd.args, atom->u.describe_args);\n+\t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n+\t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n+\t\t\terror(_(\"failed to run 'describe'\"));\n+\t\t\tv->s = xstrdup(\"\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_rtrim(&out);\n+\t\tv->s = strbuf_detach(&out, NULL);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)\n {\n@@ -1771,6 +1899,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_tag_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n@@ -1778,6 +1907,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n \t\tgrab_signature(val, deref, obj);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 6e6ec852b5..4bbba76874 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -562,6 +562,120 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n+test_expect_success 'setup for describe atom tests' '\n+\tgit init describe-repo &&\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\ttest_commit --no-tag one &&\n+\t\tgit tag tagone &&\n+\n+\t\ttest_commit --no-tag two &&\n+\t\tgit tag -a -m \"tag two\" tagtwo\n+\t)\n+'\n+\n+test_expect_success 'describe atom vs git describe' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname)\" \\\n+\t\t\trefs/tags/ >obj &&\n+\t\twhile read hash\n+\t\tdo\n+\t\t\tif desc=$(git describe $hash)\n+\t\t\tthen\n+\t\t\t\t: >expect-contains-good\n+\t\t\telse\n+\t\t\t\t: >expect-contains-bad\n+\t\t\tfi &&\n+\t\t\techo \"$hash $desc\" || return 1\n+\t\tdone expect &&\n+\t\ttest_path_exists expect-contains-good &&\n+\t\ttest_path_exists expect-contains-bad &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname) %(describe)\" \\\n+\t\t\trefs/tags/ >actual 2>err &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'describe:tags vs describe --tags' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit describe --tags >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:tags)\" \\\n+\t\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# Case 1: We have commits between HEAD and the most\n+\t\t#\t recent tag reachable from it\n+\t\ttest_commit --no-tag file &&\n+\t\tgit describe --abbrev=14 >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Make sure the hash used is atleast 14 digits long\n+\t\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\t\ttest 15 -le $(wc -c expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest tagname = $(cat actual)\n+\t)\n+'\n+\n+test_expect_success 'describe:match=... vs describe --match ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag foo\" tag-foo &&\n+\t\tgit describe --match \"*-foo\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:match=\"*-foo\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag bar\" tag-bar &&\n+\t\tgit describe --exclude \"*-bar\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:exclude=\"*-bar\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'deref with describe atom' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\ttagname\n+\t\ttagname\n+\t\ttagname\n+\n+\t\ttagtwo\n+\t\tEOF\n+\t\tgit for-each-ref --format=\"%(*describe)\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n cat >expected <<\\EOF\n heads/main\n tags/main\n-- \n2.41.0.378.g42703afc1f.dirty\n\n"},{"id":"479662","messageId":"xmqqy1jb7bow.fsf@gitster.g","threadId":"59952","inReplyTo":"20230719162424.70781-3-five231003@gmail.com","subject":"Re: [PATCH v3 2/2] ref-filter: add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-19T22:56:15Z","receivedAt":"2023-07-19T22:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> Duplicate the logic of %(describe) and friends from pretty to\n> ref-filter. In the future, this change helps in unifying both the\n> formats as ref-filter will be able to do everything that pretty is doing\n> and we can have a single interface.\n>\n> The new atom \"describe\" and its friends are equivalent to the existing\n> pretty formats with the same name.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Hariom Verma \n> Signed-off-by: Kousik Sanagavarapu \n> ---\n> Documentation/git-for-each-ref.txt | 23 +++++\n> ref-filter.c | 130 +++++++++++++++++++++++++++++\n> t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++\n> 3 files changed, 267 insertions(+)\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 2e0318770b..395daf1b22 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -258,6 +258,29 @@ ahead-behind:::\n> \tcommits ahead and behind, respectively, when comparing the output\n> \tref to the `` specified in the format.\n> \n> +describe[:options]:: Human-readable name, like\n> +\t\t link-git:git-describe[1]; empty string for\n> +\t\t undescribable commits. The `describe` string may be\n> +\t\t followed by a colon and zero or more comma-separated\n> +\t\t options.\n\nWhy do these new items formatted so differently from the previous\nones? By indenting the lines so deeply you are forcing yourself to\nwrap these lines many times. How about imitating the previous entry\nfor ahead-behind and writing this like so:\n\n describe[:]::\n A human-readable name, like linkgit:git-describe[1];\n empty string is given for an undescribable commit.\n\t\t...\n\nBy the way, there is a typo \"link-git\" above that needs to be\ncorrected.\n\nIt is curious that we support \"describe:\" (i.e. having no options,\nbut colon is still present). It may not be wrong per se, but it\nlooks strange. \"may be followed by a colon and one or more\ncomma-separated options\" would be more intuitive (I haven't seen the\nimplementation yet, so if we go that route, the implementation may\nalso need to be updated).\n\n> .... Descriptions can be inconsistent when tags\n> +\t\t are added or removed at the same time.\n\n\"at the same time\" meaning \"while the description are being\ncomputed\"? I think this was copied from 273c9901 (pretty: document\nmultiple %(describe) being inconsistent, 2021-02-28) where the\npretty placeholder for \"git log\" and friends are described, and the\nimplementation used there go one formatting element at a time,\nunlike for-each-ref that can compute a description for a given ref\njust once in populate_value() and reuse the same atom number of\ntimes in the format, each instance giving exactly the same value.\nSo I am not sure if the \"can be inconsistent\" disclaimer applies\nto the %(describe) on this side the same way. Are you sure?\n\nAs %(describe) is fairly expensive to compute, if the format string\nwants two, e.g. --format=\"%(refname) %(describe) %(describe)\", there\nshould be some effort to make these two share the same used_atom(),\nso that there will be only one \"git describe\" invocation from\npopulate_value() that lets get_ref_atom_value() reuse that result\nof a single invocation to fill the two placeholder.\n\n\n> @@ -219,6 +222,7 @@ static struct used_atom {\n> \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n> \t\t\t S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n> \t\t} signature;\n> +\t\tconst char **describe_args;\n> \t\tstruct refname_atom refname;\n> \t\tchar *head;\n> \t} u;\n\nNice and simple ;-).\n\n> +static int describe_atom_option_parser(struct strvec *args, const char **arg,\n> +\t\t\t\t struct strbuf *err)\n> +{\n> +\tconst char *argval;\n> +\tsize_t arglen = 0;\n> +\tint optval = 0;\n> +\n> +\tif (match_atom_bool_arg(*arg, \"tags\", arg, &optval)) {\n> +\t\tif (!optval)\n> +\t\t\tstrvec_push(args, \"--no-tags\");\n> +\t\telse\n> +\t\t\tstrvec_push(args, \"--tags\");\n> +\t\treturn 1;\n> +\t}\n\nOK. One thing that I hate about the split of this series into two\nsteps is that [1/2] has to be read without knowing what the expected\nuse of those two helper functions are. It was especially bad as the\nfunctions lacked any documentation on how they are supposed to be\ncalled.\n\nNow, if we go back to the implementation of match_atom_bool_arg(),\nit first called match_atom_arg_value(), which stripped the given key\n(\"tags\" in this case) from the argument being parsed, and allowed\n'=' (i.e. followed by a val), ',' (i.e. no val, but more \"key[=val]\"\nto follow), or '\\0' (i.e. end of the argument string). Anything\nelse after the matched key meant that the key did not match\n(e.g. the arg had \"tagsabcd\", which should not match \"tag\"). And it\nstored the byte position after '=' if the key was terminated with\n'=', or NULL otherwise, to signal where the optional value starts.\nmatch_atom_bool_arg() uses this and correctly translates a key\nwithout the optional [=val] part into \"true\". So the above is\ngiven, after the caller skips \"%(describe:\", things like \"tags,...\",\n\"tags=no,...\", \"tags=yes,...\" (or replace \",...\" with a NUL for the\nfinal option), and chooses between --no-tags and --tags. Sounds\ngood.\n\n> + ...\n> +\tif (match_atom_arg_value(*arg, \"exclude\", arg, &argval, &arglen)) {\n> +\t\tif (!arglen)\n> +\t\t\treturn strbuf_addf_ret(err, -1,\n> +\t\t\t\t\t _(\"value expected %s=\"),\n> +\t\t\t\t\t \"describe:exclude\");\n> +\n> +\t\tstrvec_pushf(args, \"--exclude=%.*s\", (int)arglen, argval);\n> +\t\treturn 1;\n> +\t}\n\nI would have expected that these become if/else if/.../else cascade,\ni.e.\n\n\tif (is that \"tags\"?) {\n\t} else if (is that \"abbrev\"?) {\n\t\t...\n\t} else\n\t\treturn 0; /* nothing matched */\n\treturn 1;\n\nbut I do not mind the above. Each \"block\" that matches and handles\none key looks more indenendent the way the patch was written, which\nmay be a good thing.\n\n> +\treturn 0;\n> +}\n> +\n> +static int describe_atom_parser(struct ref_format *format UNUSED,\n> +\t\t\t\tstruct used_atom *atom,\n> +\t\t\t\tconst char *arg, struct strbuf *err)\n> +{\n> +\tstruct strvec args = STRVEC_INIT;\n\nOK, parse_ref_fitler_atom() saw \"%(describe\", possibly followed by a\ncolon and zero or more comma-separated key[=val], and the location\nafter ':' (or NULL) is given to arg. Specifically, %(describe) and\n%(describe:) both pass NULL in arg.\n\n> +\tfor (;;) {\n> +\t\tint found = 0;\n> +\t\tconst char *bad_arg = NULL;\n> +\n> +\t\tif (!arg || !*arg)\n> +\t\t\tbreak;\n\nAnd we stop when there is no more key[=val].\n\n> +\t\tbad_arg = arg;\n> +\t\tfound = describe_atom_option_parser(&args, &arg, err);\n\nThis one moves arg forward and arranges the next key[=val] to be seen\nin the next iteration of this loop. Makes sense.\n\nIn the remainder of the code changes, I saw nothing strange.\nQuite well made.\n"},{"id":"479663","messageId":"xmqqjzuv5vvg.fsf@gitster.g","threadId":"59952","inReplyTo":"20230719162424.70781-2-five231003@gmail.com","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-19T23:23:15Z","receivedAt":"2023-07-19T23:24:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 59 insertions(+)\n\nNew helper functions that do not have any caller and no\ndocumentation to explain how they are supposed to be called\n(i.e. the expectation on the callers---what values they need to feed\nas parameters when they call these helpers, and the expectation by\nthe callers---what they expect to get out of the helpers once they\nreturn) makes it impossible to evaluate if they are any good [*].\n\n\tSide note. Those of you who are keen to add unit tests to\n\tthe system (Cc:ed) , do you think a patch line this one that\n\tadds a new helper function to the system, would benefit from\n\tbeing able to add a few unit tests for these otherwise\n\tunused helper functions?\n\n\tThe calls to the new functions that the unit test framework\n\twould make should serve as a good piece of interface\n\tdocumentation, showing what the callers are supposed to pass\n\tand what they expect, I guess.\n\n\tSo whatever framework we choose, it should allow adding a\n\ttest or two to this patch easily, without being too\n\tintrusive. Would that be a good and concrete evaluation\n\tcriterion?\n\nAnyway, because of that, I had to read [2/2] first and then come\nback here to review this one.\n\nThe following is my attempt to write down the contract between the\ncallers and this new helper function---please give something like\nthat to the final version. The the example below is there just to\nillustrate the level of information that would be desired to help\nfuture readers and programmers. Do not take the contents as-written\nas truth---I may have (deliberately) mixed in incorrect descriptions\n;-).\n\n/*\n * The string \"to_parse\" is expected to be a comma-separated list\n * of \"key\" or \"key=val\". If your atom allows \"key1\" and \"key2\"\n * (possibly with their values) as options, make two calls to this\n * funtion, passing \"key1\" in candiate and then passing \"key2\" in\n * candidate.\n *\n * The function Returns true ONLY when the to_parse string begins\n * with the candidate key, possibly followed by its value (valueless\n * key-only entries are allowed in the comman-separated list).\n * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and\n * the function returns false.\n *\n * *valuestart will point at the byte after '=' (i.e. the beginning\n * of the value), and the number of bytes in the value will be set\n * to *valuelen.\n * A key-only entry results in *valuestart set to NULL and *valuelen\n * set to 0.\n * *end will point at the next key[=val] in the comma-separated list\n * or NULL when the list ran out.\n */\n\n> +static int match_atom_arg_value(const char *to_parse, const char *candidate,\n> +\t\t\t\tconst char **end, const char **valuestart,\n> +\t\t\t\tsize_t *valuelen)\n> +{\n> +\tconst char *atom;\n> +\n> +\tif (!(skip_prefix(to_parse, candidate, &atom)))\n> +\t\treturn 0;\n> +\tif (valuestart) {\n\nAs far as I saw, no callers pass NULL to valuestart. Getting rid of\nthis if() statement and always entering its body would clarify what\nis going on, I think.\n\n> +\t\tif (*atom == '=') {\n> +\t\t\t*valuestart = atom + 1;\n> +\t\t\t*valuelen = strcspn(*valuestart, \",\\0\");\n> +\t\t\tatom = *valuestart + *valuelen;\n> +\t\t} else {\n> +\t\t\tif (*atom != ',' && *atom != '\\0')\n> +\t\t\t\treturn 0;\n> +\t\t\t*valuestart = NULL;\n> +\t\t\t*valuelen = 0;\n> +\t\t}\n> +\t}\n> +\tif (*atom == ',') {\n> +\t\t*end = atom + 1;\n> +\t\treturn 1;\n> +\t}\n> +\tif (*atom == '\\0') {\n> +\t\t*end = atom;\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n\n/*\n * Write something similar to document the contract between the caller\n * and this function here.\n */\n> +static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n> +\t\t\t\tconst char **end, int *val)\n> +{\n\nThanks.\n"},{"id":"479668","messageId":"xmqqcz0n40pu.fsf@gitster.g","threadId":"59952","inReplyTo":"xmqqjzuv5vvg.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T05:21:33Z","receivedAt":"2023-07-20T05:22:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +static int match_atom_arg_value(const char *to_parse, const char *candidate,\n>> +\t\t\t\tconst char **end, const char **valuestart,\n>> +\t\t\t\tsize_t *valuelen)\n>> +{\n>> +\tconst char *atom;\n>> +\n>> +\tif (!(skip_prefix(to_parse, candidate, &atom)))\n>> +\t\treturn 0;\n>> +\tif (valuestart) {\n>\n> As far as I saw, no callers pass NULL to valuestart. Getting rid of\n> this if() statement and always entering its body would clarify what\n> is going on, I think.\n\nSpecifically, ...\n\n>> +\t\tif (*atom == '=') {\n>> +\t\t\t*valuestart = atom + 1;\n>> +\t\t\t*valuelen = strcspn(*valuestart, \",\\0\");\n>> +\t\t\tatom = *valuestart + *valuelen;\n>> +\t\t} else {\n>> +\t\t\tif (*atom != ',' && *atom != '\\0')\n>> +\t\t\t\treturn 0;\n>> +\t\t\t*valuestart = NULL;\n>> +\t\t\t*valuelen = 0;\n>> +\t\t}\n>> +\t}\n>> +\tif (*atom == ',') {\n>> +\t\t*end = atom + 1;\n>> +\t\treturn 1;\n>> +\t}\n>> +\tif (*atom == '\\0') {\n>> +\t\t*end = atom;\n>> +\t\treturn 1;\n>> +\t}\n>> +\treturn 0;\n>> +}\n\n... I think the body of the function would become easier to read if\nwritten like so:\n\n\tif (!skip_prefix(to_parse, candidate, &atom))\n\t\treturn 0; /* definitely not \"candidate\" */\n\n\tif (*atom == '=') {\n\t\t/* we just saw \"candidate=\" */\n\t\t*valuestart = atom + 1;\n atom = strchrnul(*valuestart, ',');\n\t\t*valuelen = atom - *valuestart;\n\t} else if (*atom != ',' && *atom != '\\0') {\n \t /* key begins with \"candidate\" but has more chars */\n\t\treturn 0;\n\t} else {\n \t/* just \"candidate\" without \"=val\" */\n\t\t*valuestart = NULL;\n\t\t*valuelen = 0;\n\t}\n\n /* atom points at either the ',' or NUL after this key[=val] */\n\tif (*atom == ',')\n\t\tatom++;\n\telse if (*atom)\n\t\tBUG(\"should not happen\");\n\n\t*end = atom;\n\treturn 1;\n\nas it is clear that *valuestart, *valuelen, and *end are not touched\nwhen the function returns 0 and they are all filled when the function\nreturns 1.\n\nAlso, avoid passing \",\\0\" to strcspn(); its effect is exactly the\nsame as passing \",\", and at that point you are better off using\nstrchnul().\n\nThanks.\n\n"},{"id":"479672","messageId":"ZLlmXNt2crTEIXLg@five231003","threadId":"59952","inReplyTo":"xmqqjzuv5vvg.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-20T16:52:44Z","receivedAt":"2023-07-20T16:53:03Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Wed, Jul 19, 2023 at 04:23:15PM -0700, Junio C Hamano wrote:\n> Kousik Sanagavarapu writes:\n> \n> > ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 59 insertions(+)\n> \n> New helper functions that do not have any caller and no\n> documentation to explain how they are supposed to be called\n> (i.e. the expectation on the callers---what values they need to feed\n> as parameters when they call these helpers, and the expectation by\n> the callers---what they expect to get out of the helpers once they\n> return) makes it impossible to evaluate if they are any good [*].\n> \n> \tSide note. Those of you who are keen to add unit tests to\n> \tthe system (Cc:ed) , do you think a patch line this one that\n> \tadds a new helper function to the system, would benefit from\n> \tbeing able to add a few unit tests for these otherwise\n> \tunused helper functions?\n> \n> \tThe calls to the new functions that the unit test framework\n> \twould make should serve as a good piece of interface\n> \tdocumentation, showing what the callers are supposed to pass\n> \tand what they expect, I guess.\n> \n> \tSo whatever framework we choose, it should allow adding a\n> \ttest or two to this patch easily, without being too\n> \tintrusive. Would that be a good and concrete evaluation\n> \tcriterion?\n> \n> Anyway, because of that, I had to read [2/2] first and then come\n> back here to review this one.\n> \n> The following is my attempt to write down the contract between the\n> callers and this new helper function---please give something like\n> that to the final version. The the example below is there just to\n> illustrate the level of information that would be desired to help\n> future readers and programmers. Do not take the contents as-written\n> as truth---I may have (deliberately) mixed in incorrect descriptions\n> ;-).\n\nI'll spot them---if there are any ;).\n\n> \n> /*\n> * The string \"to_parse\" is expected to be a comma-separated list\n> * of \"key\" or \"key=val\". If your atom allows \"key1\" and \"key2\"\n> * (possibly with their values) as options, make two calls to this\n> * funtion, passing \"key1\" in candiate and then passing \"key2\" in\n> * candidate.\n> *\n> * The function Returns true ONLY when the to_parse string begins\n> * with the candidate key, possibly followed by its value (valueless\n> * key-only entries are allowed in the comman-separated list).\n> * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and\n> * the function returns false.\n> *\n> * *valuestart will point at the byte after '=' (i.e. the beginning\n> * of the value), and the number of bytes in the value will be set\n> * to *valuelen.\n> * A key-only entry results in *valuestart set to NULL and *valuelen\n> * set to 0.\n> * *end will point at the next key[=val] in the comma-separated list\n> * or NULL when the list ran out.\n> */\n> \n> > +static int match_atom_arg_value(const char *to_parse, const char *candidate,\n> > +\t\t\t\tconst char **end, const char **valuestart,\n> > +\t\t\t\tsize_t *valuelen)\n> > +{\n> > +\tconst char *atom;\n> > +\n> > +\tif (!(skip_prefix(to_parse, candidate, &atom)))\n> > +\t\treturn 0;\n> > +\tif (valuestart) {\n> \n> As far as I saw, no callers pass NULL to valuestart. Getting rid of\n> this if() statement and always entering its body would clarify what\n> is going on, I think.\n> \n> > +\t\tif (*atom == '=') {\n> > +\t\t\t*valuestart = atom + 1;\n> > +\t\t\t*valuelen = strcspn(*valuestart, \",\\0\");\n> > +\t\t\tatom = *valuestart + *valuelen;\n> > +\t\t} else {\n> > +\t\t\tif (*atom != ',' && *atom != '\\0')\n> > +\t\t\t\treturn 0;\n> > +\t\t\t*valuestart = NULL;\n> > +\t\t\t*valuelen = 0;\n> > +\t\t}\n> > +\t}\n> > +\tif (*atom == ',') {\n> > +\t\t*end = atom + 1;\n> > +\t\treturn 1;\n> > +\t}\n> > +\tif (*atom == '\\0') {\n> > +\t\t*end = atom;\n> > +\t\treturn 1;\n> > +\t}\n> > +\treturn 0;\n> > +}\n> \n> /*\n> * Write something similar to document the contract between the caller\n> * and this function here.\n> */\n> > +static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n> > +\t\t\t\tconst char **end, int *val)\n> > +{\n\nI'll make these changes in the re-rolled version. I've also read your\nreply to this email with the changes in `match_atom_arg_value()`. I'll\nadd them too.\n\nGoing off in a tangent here---In yesterday's review club which discussed\nthe %(decorate:) patch[1], Glen suggested the possibility of\nhaving a single kind of a framework (used this word very loosely here)\nfor parsing these multiple options since we are beginning to see them so\noften (might also help new formats which maybe added in the future). The\nfact that this wasn't done already says something about its difficulty\nas Jacob mentioned yesterday. The difficulty being we don't exactly know\nwhich options to parse as they differ from format to format.\n\nChristian, Hariom and I had a similar discussion about refactoring these\nhelper functions that are already there in pretty (`match_placeholder_*()`)\nso that they can be used here.\n\nOne example of this usage of functions from pretty is done when making\nref-filter support trailers.\n\n[1]: https://lore.kernel.org/git/20230715160730.4046-1-andy.koppe@gmail.com/\n\nThanks\n"},{"id":"479673","messageId":"kl6lzg3qzdhn.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59952","inReplyTo":"xmqqjzuv5vvg.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-07-20T17:42:12Z","receivedAt":"2023-07-20T17:42:17Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> New helper functions that do not have any caller and no\n> documentation to explain how they are supposed to be called\n> (i.e. the expectation on the callers---what values they need to feed\n> as parameters when they call these helpers, and the expectation by\n> the callers---what they expect to get out of the helpers once they\n> return) makes it impossible to evaluate if they are any good [*].\n\nAgreed.\n\n> \tSide note. Those of you who are keen to add unit tests to\n> \tthe system (Cc:ed) , do you think a patch line this one that\n> \tadds a new helper function to the system, would benefit from\n> \tbeing able to add a few unit tests for these otherwise\n> \tunused helper functions?\n\nAbsolutely. As a rule, we should strive to test all of our changes as\nthey are introduced. With our current shell-based testing, this means\nthat we have to add callers (either via a builtin or test-helper), but\nIMO a unit test framework would serve this purpose even better.\n\n> \tThe calls to the new functions that the unit test framework\n> \twould make should serve as a good piece of interface\n> \tdocumentation, showing what the callers are supposed to pass\n> \tand what they expect, I guess.\n\nAgreed, and as documentation, unit tests can be easier to read, since\nthey can include only the relevant details.\n\n> \tSo whatever framework we choose, it should allow adding a\n> \ttest or two to this patch easily, without being too\n> \tintrusive. Would that be a good and concrete evaluation\n> \tcriterion?\n\nPerhaps, but the biggest blocker to adding a unit tests is whether the\nsource file itself is amenable to being unit tested (e.g. does it depend\non global state? does it compile easily?). Once that is in place, I\ncan't imagine that there would be a sensible unit test framework that\ndoesn't make it easy to add tests to a patch like this.\n"},{"id":"479674","messageId":"xmqq351i4g6s.fsf@gitster.g","threadId":"59952","inReplyTo":"ZLlmXNt2crTEIXLg@five231003","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T17:59:39Z","receivedAt":"2023-07-20T17:59:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> Going off in a tangent here---In yesterday's review club which discussed\n> the %(decorate:) patch[1], Glen suggested the possibility of\n> having a single kind of a framework (used this word very loosely here)\n> for parsing these multiple options since we are beginning to see them so\n> often (might also help new formats which maybe added in the future). The\n> fact that this wasn't done already says something about its difficulty\n> as Jacob mentioned yesterday. The difficulty being we don't exactly know\n> which options to parse as they differ from format to format.\n\nYes, while writing the sample in-code documentation for the\nmatch_atom_arg_value() function, I found its interface to force the\ncallers to do \"parse if it is 'tags'; else parse if it is 'abbrev';\nand so on\" hard to use. I wondered if the primitive to parse these\nshould be modeled after config.c:parse_config_key(), i.e. the helper\nfunction takes the input and returns the split point and lengths of\nthe components it finds in the return parameter.\n\nAs the contract between the caller and the callee is that the caller\npasses the beginning of \"key1[=val1],key2[=val2],...\", the interface\nmay look like\n\n\tint parse_cskv(const char *to_parse,\n\t\t const char **key, size_t *keylen,\n\t\t const char **val, size_t *vallen,\n\t\t const char **end);\n\nand would be used like so:\n\n\tconst char *cskv = \"key1,key2=val2\";\n\tconst char *key, *val;\n\tsize_t keylen, vallen;\n\n\twhile (parse_cskv(cskv, &key, &keylen, &val, &vallen, &cskv) {\n\t\tif (!val)\n\t\t\tprintf(\"valueless key '%.*s'\\n\",\n\t\t\t (int)keylen, key);\n\t\telse\n\t\t\tprintf(\"key-value pair '%.*s=%.*s'\\n\",\n\t\t\t (int)keylen, key, (int)vallen, val);\n\t}\n\tif (*cskv)\n\t\tfprintf(stderr, \"error - trailing garbage seen '%s'\\n\",\n\t\t\tcskv);\n\nThe helper's contract to the caller may look like this:\n\n - It expects the string to be \"key\" or \"key=val\" followed by ',' or\n '\\0' (the latter ends the input, i.e. the last element of comma\n separated list). The out variables key, val, keylen, vallen are\n used to convey to the caller where key and val are found and how\n long they are.\n\n - If it is a valueless \"key\", the out variable val is set to NULL.\n\n - The out variable end is updated to point at one byte after the\n element that has just been parsed.\n\nThe need for the caller to check against the list of keys it knows\nabout in the loop still exists, but the parser may become simpler\nthat way. I dunno.\n\n"},{"id":"479681","messageId":"xmqqzg3q1g2y.fsf@gitster.g","threadId":"59952","inReplyTo":"kl6lzg3qzdhn.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T20:30:13Z","receivedAt":"2023-07-20T20:30:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n>> \tSo whatever framework we choose, it should allow adding a\n>> \ttest or two to this patch easily, without being too\n>> \tintrusive. Would that be a good and concrete evaluation\n>> \tcriterion?\n>\n> Perhaps, but the biggest blocker to adding a unit tests is whether the\n> source file itself is amenable to being unit tested (e.g. does it depend\n> on global state? does it compile easily?).\n\nPerhaps. \n\nNow would this particular example, the change to ref-filter.c file,\nbe a reasonable guinea-pig test case for candidate test frameworks\nto add tests for these two helper functions? They are pretty-much\nplain vanilla string manipulation functions that does not depend too\nmany things that are specific to Git. They may use helpers we\nwrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(),\nbut they shouldn't depend on the program start-up sequence,\ndiscovering repositories, installing at-exit handers, and other\nstuff. It was why I wondered if it can be used as a good evaluation\ncriterion---if a test framework cannot easily add tests while this\npatch was being proposed in a non-intrusive way to demonstrate how\nthese two functions are supposed to work and to protect their\nimplementations from future breakage, it would not be all that\nuseful, I would imagine.\n"},{"id":"479699","messageId":"xmqqr0p219ib.fsf@gitster.g","threadId":"59952","inReplyTo":"20230719162424.70781-1-five231003@gmail.com","subject":"Re: [PATCH v3 0/2] Add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T22:52:12Z","receivedAt":"2023-07-20T22:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> PATCH 1/2 - Left unchanged expect for small changes in the commit\n> \t message for more clarity.\n>\n> PATCH 2/2 - We now parse the arguments in a seperate function\n> \t `describe_atom_option_parser()` call this in\n> \t `describe_atom_parser()` instead to populate\n> \t `atom->u.describe_args`. This splitting of the function\n> \t helps err at the right places.\n\nThis topic may be getting rerolled but from the CI logs,\ncomparing \n\n * https://github.com/git/git/actions/runs/5603242871 (seen at\n 77ba682) that passes the tests\n\n * https://github.com/git/git/actions/runs/5605480104 (seen at\n 29f0316) that breaks linux-gcc (ubuntu-20.04) at t6300 [*]\n\noutput from \"git shortlog --no-merges 77ba682..29f0316\" [*] makes us\nsuspect that this topic may be the culprit of the recent breakage.\n\nThe linux-gcc job is where we force the initial branch name to be\n'main' and not 'master', so if your tests assume that the initial &\nprimary branch name is 'master', that may be something you need to\nfix.\n\nThanks.\n\n[Reference]\n * https://github.com/git/git/actions/runs/5605480104/job/15186229680\n\n * git shortlog --no-merges 77ba682..29f0316\nAlex Henrie (1):\n sequencer: finish parsing the todo list despite an invalid first line\n\nBeat Bolli (1):\n trace2: fix a comment\n\nJunio C Hamano (4):\n short help: allow multi-line opthelp\n remote: simplify \"remote add --tags\" help text\n short help: allow a gap smaller than USAGE_GAP\n ###\n\nKousik Sanagavarapu (2):\n ref-filter: add multiple-option parsing functions\n ref-filter: add new \"describe\" atom\n\n\n"},{"id":"479700","messageId":"xmqqjzuu18oe.fsf@gitster.g","threadId":"59952","inReplyTo":"xmqqr0p219ib.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-20T23:10:09Z","receivedAt":"2023-07-20T23:10:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The linux-gcc job is where we force the initial branch name to be\n> 'main' and not 'master', so if your tests assume that the initial &\n> primary branch name is 'master', that may be something you need to\n> fix.\n\nPerhaps something along the line of the attached patch?\n\nThe primary test repository t6300 uses is aware of the \"problem\"\nwhere the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\nto 'main' and hacks it around by using\n\n\tgit branch -M main\n\nas one of the first things it does, to _force_ the primary branch\nname always to 'main', whether the tester's environment forces \"git\"\nto start with 'main' or 'master', and existing tests in the script\nrelies on 'main' being the primary branch.\n\nBut your tests are done in a repository newly created with your own\n\"git init\", so depending on the tester's environment, the primary\nbranch may be 'master' or 'main'. The way your new tests are\nwritten, however, things will fail if \"refs/heads/master\" is not the\nprimary branch.\n\n\n t/t6300-for-each-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh\nindex 4bbba76874..489f4d9186 100755\n--- c/t/t6300-for-each-ref.sh\n+++ w/t/t6300-for-each-ref.sh\n@@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' '\n '\n \n test_expect_success 'setup for describe atom tests' '\n-\tgit init describe-repo &&\n+\tgit init -b master describe-repo &&\n \t(\n \t\tcd describe-repo &&\n \n"},{"id":"479705","messageId":"ZLoG6a7tG-HisBhq@five231003","threadId":"59952","inReplyTo":"xmqqjzuu18oe.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-21T04:17:45Z","receivedAt":"2023-07-21T04:18:06Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Thu, Jul 20, 2023 at 04:10:09PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > The linux-gcc job is where we force the initial branch name to be\n> > 'main' and not 'master', so if your tests assume that the initial &\n> > primary branch name is 'master', that may be something you need to\n> > fix.\n> \n> Perhaps something along the line of the attached patch?\n> \n> The primary test repository t6300 uses is aware of the \"problem\"\n> where the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> to 'main' and hacks it around by using\n> \n> \tgit branch -M main\n> \n> as one of the first things it does, to _force_ the primary branch\n> name always to 'main', whether the tester's environment forces \"git\"\n> to start with 'main' or 'master', and existing tests in the script\n> relies on 'main' being the primary branch.\n> \n> But your tests are done in a repository newly created with your own\n> \"git init\", so depending on the tester's environment, the primary\n> branch may be 'master' or 'main'. The way your new tests are\n> written, however, things will fail if \"refs/heads/master\" is not the\n> primary branch.\n> \n\nI see. I looked at the trash directory by doing -v -i -d when running\nthese describe tests and was under the impression that it is always\nmaster.I guess I should have had a bigger view of things.\n\n> t/t6300-for-each-ref.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh\n> index 4bbba76874..489f4d9186 100755\n> --- c/t/t6300-for-each-ref.sh\n> +++ w/t/t6300-for-each-ref.sh\n> @@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' '\n> '\n> \n> test_expect_success 'setup for describe atom tests' '\n> -\tgit init describe-repo &&\n> +\tgit init -b master describe-repo &&\n> \t(\n> \t\tcd describe-repo &&\n\nI'll add this to the re-rolled version.\n\nThanks\n"},{"id":"479761","messageId":"kl6lr0p1yvc1.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59952","inReplyTo":"xmqqzg3q1g2y.fsf@gitster.g","subject":"Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-07-21T18:26:38Z","receivedAt":"2023-07-21T18:26:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> Glen Choo writes:\n>\n>>> \tSo whatever framework we choose, it should allow adding a\n>>> \ttest or two to this patch easily, without being too\n>>> \tintrusive. Would that be a good and concrete evaluation\n>>> \tcriterion?\n>>\n>> Perhaps, but the biggest blocker to adding a unit tests is whether the\n>> source file itself is amenable to being unit tested (e.g. does it depend\n>> on global state? does it compile easily?).\n>\n> Now would this particular example, the change to ref-filter.c file,\n> be a reasonable guinea-pig test case for candidate test frameworks\n> to add tests for these two helper functions? They are pretty-much\n> plain vanilla string manipulation functions that does not depend too\n> many things that are specific to Git.\n\nAh, yes. This would be close-to-ideal candidate then.\n\n> They may use helpers we\n> wrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(),\n> but they shouldn't depend on the program start-up sequence,\n> discovering repositories, installing at-exit handers, and other\n> stuff. It was why I wondered if it can be used as a good evaluation\n> criterion---if a test framework cannot easily add tests while this\n> patch was being proposed in a non-intrusive way to demonstrate how\n> these two functions are supposed to work and to protect their\n> implementations from future breakage, it would not be all that\n> useful, I would imagine.\n\nThe current thinking among Googlers is that we won't remove the helpers\nfrom library code. Git will either provide them, e.g. via Calvin's\ngit-std-lib RFC [1], or we will provide ways for callers to bring their\nown implementation (like trace2 or exit, since it doesn't necessarily\nmake sense to use Git's implementation). So yes, the test framework\nshould be able to support this sort of compilation pattern. I'm not sure\nhow much test frameworks differ in this regard, maybe Josh has some\ninsight here.\n\n[1] https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com\n"},{"id":"479784","messageId":"20230723162717.68123-1-five231003@gmail.com","threadId":"59952","inReplyTo":"20230719162424.70781-1-five231003@gmail.com","subject":"[PATCH v4 0/2] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-23T16:19:57Z","receivedAt":"2023-07-23T16:28:34Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\nThanks for the review on the previous version.\n\nPATCH 1/2 - Added comments to explain what the helper functions do and\n\t also fixed a nit where config.h should be included\n\t here and not the subsequent commit.\n\nPATCH 2/2 - Changed the formatting on Documentation to follow what was\n\t in the file. Also, the statement\n\t\t\n\t\t\"Descriptions can be insconsistent when tags are added\n\t\tor removed at the same time\"\n\n\t is not true in ref-filter's case as,\n\n\t\t$ time git for-each-ref --format=\"%(describe)\" refs/\t\t \n\t\treal\t0m19.936s\n\t\tuser\t0m11.488s\n\t\tsys\t0m7.915s\n\n\n\t\t$ time git for-each-ref --format=\"%(describe) %(describe)\" refs/\t\n\t\treal\t0m19.502s\n\t\tuser\t0m11.653s\n\t\tsys\t0m7.623s\n\n\t I also added a test to check that we err on bad describe\n\t args.\n\nKousik Sanagavarapu (2):\n ref-filter: add multiple-option parsing functions\n ref-filter: add new \"describe\" atom\n\n Documentation/git-for-each-ref.txt | 23 +++\n ref-filter.c | 230 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 138 +++++++++++++++++\n 3 files changed, 391 insertions(+)\n\nRange-diff against v3:\n\n1: 08f3be1631 ! 1: 2914bd58ec ref-filter: add multiple-option parsing functions\n @@ Commit message\n are going to add a new %(describe) atom in a subsequent commit where we\n parse options like tags= or match= given to it.\n \n + Helped-by: Junio C Hamano \n Mentored-by: Christian Couder \n Mentored-by: Hariom Verma \n Signed-off-by: Kousik Sanagavarapu \n \n ## ref-filter.c ##\n +@@\n + #include \"git-compat-util.h\"\n + #include \"environment.h\"\n + #include \"gettext.h\"\n ++#include \"config.h\"\n + #include \"gpg-interface.h\"\n + #include \"hex.h\"\n + #include \"parse-options.h\"\n @@ ref-filter.c: static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)\n return -1;\n }\n \n ++/*\n ++ * Parse option of name \"candidate\" in the option string \"to_parse\" of\n ++ * the form\n ++ *\n ++ * \"candidate1[=val1],candidate2[=val2],candidate3[=val3],...\"\n ++ *\n ++ * The remaining part of \"to_parse\" is stored in \"end\" (if we are\n ++ * parsing the last candidate, then this is NULL) and the value of\n ++ * the candidate is stored in \"valuestart\" and its length in \"valuelen\",\n ++ * that is the portion after \"=\". Since it is possible for a \"candidate\"\n ++ * to not have a value, in such cases, \"valuestart\" is set to point to\n ++ * NULL and \"valuelen\" to 0.\n ++ *\n ++ * The function returns 1 on success. It returns 0 if we don't find\n ++ * \"candidate\" in \"to_parse\" or we find \"candidate\" but it is followed\n ++ * by more chars (for example, \"candidatefoo\"), that is, we don't find\n ++ * an exact match.\n ++ *\n ++ * This function only does the above for one \"candidate\" at a time. So\n ++ * it has to be called each time trying to parse a \"candidate\" in the\n ++ * option string \"to_parse\".\n ++ */\n +static int match_atom_arg_value(const char *to_parse, const char *candidate,\n + const char **end, const char **valuestart,\n + size_t *valuelen)\n +{\n + const char *atom;\n +\n -+ if (!(skip_prefix(to_parse, candidate, &atom)))\n ++ if (!skip_prefix(to_parse, candidate, &atom))\n ++ return 0; /* definitely not \"candidate\" */\n ++\n ++ if (*atom == '=') {\n ++ /* we just saw \"candidate=\" */\n ++ *valuestart = atom + 1;\n ++ atom = strchrnul(*valuestart, ',');\n ++ *valuelen = atom - *valuestart;\n ++ } else if (*atom != ',' && *atom != '\\0') {\n ++ /* key begins with \"candidate\" but has more chars */\n + return 0;\n -+ if (valuestart) {\n -+ if (*atom == '=') {\n -+ *valuestart = atom + 1;\n -+ *valuelen = strcspn(*valuestart, \",\\0\");\n -+ atom = *valuestart + *valuelen;\n -+ } else {\n -+ if (*atom != ',' && *atom != '\\0')\n -+ return 0;\n -+ *valuestart = NULL;\n -+ *valuelen = 0;\n -+ }\n -+ }\n -+ if (*atom == ',') {\n -+ *end = atom + 1;\n -+ return 1;\n -+ }\n -+ if (*atom == '\\0') {\n -+ *end = atom;\n -+ return 1;\n ++ } else {\n ++ /* just \"candidate\" without \"=val\" */\n ++ *valuestart = NULL;\n ++ *valuelen = 0;\n + }\n -+ return 0;\n ++\n ++ /* atom points at either the ',' or NUL after this key[=val] */\n ++ if (*atom == ',')\n ++ atom++;\n ++ else if (*atom)\n ++ BUG(\"Why is *atom not NULL yet?\");\n ++\n ++ *end = atom;\n ++ return 1;\n +}\n +\n ++/*\n ++ * Parse boolean option of name \"candidate\" in the option list \"to_parse\"\n ++ * of the form\n ++ *\n ++ * \"candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],...\"\n ++ *\n ++ * The remaining part of \"to_parse\" is stored in \"end\" (if we are parsing\n ++ * the last candidate, then this is NULL) and the value (if given) is\n ++ * parsed and stored in \"val\", so \"val\" always points to either 0 or 1.\n ++ * If the value is not given, then \"val\" is set to point to 1.\n ++ *\n ++ * The boolean value is parsed using \"git_parse_maybe_bool()\", so the\n ++ * accepted values are\n ++ *\n ++ * to set true - \"1\", \"yes\", \"true\"\n ++ * to set false - \"0\", \"no\", \"false\"\n ++ *\n ++ * This function returns 1 on success. It returns 0 when we don't find\n ++ * an exact match for \"candidate\" or when the boolean value given is\n ++ * not valid.\n ++ */\n +static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n + const char **end, int *val)\n +{\n2: 742a79113c ! 2: 77a2a56520 ref-filter: add new \"describe\" atom\n @@ Commit message\n The new atom \"describe\" and its friends are equivalent to the existing\n pretty formats with the same name.\n \n + Helped-by: Junio C Hamano \n Mentored-by: Christian Couder \n Mentored-by: Hariom Verma \n Signed-off-by: Kousik Sanagavarapu \n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n commits ahead and behind, respectively, when comparing the output\n ref to the `` specified in the format.\n \n -+describe[:options]:: Human-readable name, like\n -+ link-git:git-describe[1]; empty string for\n -+ undescribable commits. The `describe` string may be\n -+ followed by a colon and zero or more comma-separated\n -+ options. Descriptions can be inconsistent when tags\n -+ are added or removed at the same time.\n ++describe[:options]::\n ++ A human-readable name, like linkgit:git-describe[1];\n ++ empty string for undescribable commits. The `describe` string may\n ++ be followed by a colon and one or more comma-separated options.\n ++\n +--\n -+tags=;; Instead of only considering annotated tags, consider\n -+ lightweight tags as well; see the corresponding option\n -+ in linkgit:git-describe[1] for details.\n -+abbrev=;; Use at least hexadecimal digits; see\n -+ the corresponding option in linkgit:git-describe[1]\n -+ for details.\n -+match=;; Only consider tags matching the given `glob(7)` pattern,\n -+ excluding the \"refs/tags/\" prefix; see the corresponding\n -+ option in linkgit:git-describe[1] for details.\n -+exclude=;; Do not consider tags matching the given `glob(7)`\n -+ pattern, excluding the \"refs/tags/\" prefix; see the\n -+ corresponding option in linkgit:git-describe[1] for\n -+ details.\n ++tags=;;\n ++ Instead of only considering annotated tags, consider\n ++ lightweight tags as well; see the corresponding option in\n ++ linkgit:git-describe[1] for details.\n ++abbrev=;;\n ++ Use at least hexadecimal digits; see the corresponding\n ++ option in linkgit:git-describe[1] for details.\n ++match=;;\n ++ Only consider tags matching the given `glob(7)` pattern,\n ++ excluding the \"refs/tags/\" prefix; see the corresponding option\n ++ in linkgit:git-describe[1] for details.\n ++exclude=;;\n ++ Do not consider tags matching the given `glob(7)` pattern,\n ++ excluding the \"refs/tags/\" prefix; see the corresponding option\n ++ in linkgit:git-describe[1] for details.\n +--\n +\n In addition to the above, for commit and tag objects, the header\n @@ Documentation/git-for-each-ref.txt: ahead-behind:::\n \n ## ref-filter.c ##\n @@\n - #include \"git-compat-util.h\"\n - #include \"environment.h\"\n - #include \"gettext.h\"\n -+#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct\n +\n + for (;;) {\n + int found = 0;\n -+ const char *bad_arg = NULL;\n ++ const char *bad_arg = arg;\n +\n + if (!arg || !*arg)\n + break;\n +\n -+ bad_arg = arg;\n + found = describe_atom_option_parser(&args, &arg, err);\n + if (found < 0)\n + return found;\n -+ if (!found) {\n -+ if (bad_arg && *bad_arg)\n -+ return err_bad_arg(err, \"describe\", bad_arg);\n -+ break;\n -+ }\n ++ if (!found)\n ++ return err_bad_arg(err, \"describe\", bad_arg);\n + }\n + atom->u.describe_args = strvec_detach(&args);\n + return 0;\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n '\n \n +test_expect_success 'setup for describe atom tests' '\n -+ git init describe-repo &&\n ++ git init -b master describe-repo &&\n + (\n + cd describe-repo &&\n +\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n + test_cmp expect actual\n + )\n +'\n ++\n ++test_expect_success 'err on bad describe atom arg' '\n ++ (\n ++ cd describe-repo &&\n ++\n ++ # The bad arg is the only arg passed to describe atom\n ++ cat >expect <<-\\EOF &&\n ++ fatal: unrecognized %(describe) argument: baz\n ++ EOF\n ++ ! git for-each-ref --format=\"%(describe:baz)\" \\\n ++ refs/heads/master 2>actual &&\n ++ test_cmp expect actual &&\n ++\n ++ # The bad arg is in the middle of the option string\n ++ # passed to the describe atom\n ++ cat >expect <<-\\EOF &&\n ++ fatal: unrecognized %(describe) argument: qux=1,abbrev=14\n ++ EOF\n ++ ! git for-each-ref \\\n ++ --format=\"%(describe:tags,qux=1,abbrev=14)\" \\\n ++ ref/heads/master 2>actual &&\n ++ test_cmp expect actual\n ++ )\n ++'\n +\n cat >expected <<\\EOF\n heads/main\n\n-- \n2.41.0.396.g9ab76b0018\n\n"},{"id":"479785","messageId":"20230723162717.68123-2-five231003@gmail.com","threadId":"59952","inReplyTo":"20230723162717.68123-1-five231003@gmail.com","subject":"[PATCH v4 1/2] ref-filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-23T16:19:58Z","receivedAt":"2023-07-23T16:28:43Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The functions\n\n\tmatch_placeholder_arg_value()\n\tmatch_placeholder_bool_arg()\n\nwere added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\nwith explicit value, 2019-01-29) to parse multiple options in an\nargument to --pretty. For example,\n\n\tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n\nwill output all the trailers matching the key and seperates them by\na comma followed by a space per commit.\n\nAdd similar functions,\n\n\tmatch_atom_arg_value()\n\tmatch_atom_bool_arg()\n\nin ref-filter.\n\nThere is no atom yet that can use these functions in ref-filter, but we\nare going to add a new %(describe) atom in a subsequent commit where we\nparse options like tags= or match= given to it.\n\nHelped-by: Junio C Hamano \nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 105 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0f3df132b8..8d5f85e0a7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1,6 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n+#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n@@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)\n \treturn -1;\n }\n \n+/*\n+ * Parse option of name \"candidate\" in the option string \"to_parse\" of\n+ * the form\n+ *\n+ *\t\"candidate1[=val1],candidate2[=val2],candidate3[=val3],...\"\n+ *\n+ * The remaining part of \"to_parse\" is stored in \"end\" (if we are\n+ * parsing the last candidate, then this is NULL) and the value of\n+ * the candidate is stored in \"valuestart\" and its length in \"valuelen\",\n+ * that is the portion after \"=\". Since it is possible for a \"candidate\"\n+ * to not have a value, in such cases, \"valuestart\" is set to point to\n+ * NULL and \"valuelen\" to 0.\n+ *\n+ * The function returns 1 on success. It returns 0 if we don't find\n+ * \"candidate\" in \"to_parse\" or we find \"candidate\" but it is followed\n+ * by more chars (for example, \"candidatefoo\"), that is, we don't find\n+ * an exact match.\n+ *\n+ * This function only does the above for one \"candidate\" at a time. So\n+ * it has to be called each time trying to parse a \"candidate\" in the\n+ * option string \"to_parse\".\n+ */\n+static int match_atom_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, const char **valuestart,\n+\t\t\t\tsize_t *valuelen)\n+{\n+\tconst char *atom;\n+\n+\tif (!skip_prefix(to_parse, candidate, &atom))\n+\t\treturn 0; /* definitely not \"candidate\" */\n+\n+\tif (*atom == '=') {\n+\t\t/* we just saw \"candidate=\" */\n+\t\t*valuestart = atom + 1;\n+\t\tatom = strchrnul(*valuestart, ',');\n+\t\t*valuelen = atom - *valuestart;\n+\t} else if (*atom != ',' && *atom != '\\0') {\n+\t\t/* key begins with \"candidate\" but has more chars */\n+\t\treturn 0;\n+\t} else {\n+\t\t/* just \"candidate\" without \"=val\" */\n+\t\t*valuestart = NULL;\n+\t\t*valuelen = 0;\n+\t}\n+\n+\t/* atom points at either the ',' or NUL after this key[=val] */\n+\tif (*atom == ',')\n+\t\tatom++;\n+\telse if (*atom)\n+\t\tBUG(\"Why is *atom not NULL yet?\");\n+\n+\t*end = atom;\n+\treturn 1;\n+}\n+\n+/*\n+ * Parse boolean option of name \"candidate\" in the option list \"to_parse\"\n+ * of the form\n+ *\n+ *\t\"candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],...\"\n+ *\n+ * The remaining part of \"to_parse\" is stored in \"end\" (if we are parsing\n+ * the last candidate, then this is NULL) and the value (if given) is\n+ * parsed and stored in \"val\", so \"val\" always points to either 0 or 1.\n+ * If the value is not given, then \"val\" is set to point to 1.\n+ *\n+ * The boolean value is parsed using \"git_parse_maybe_bool()\", so the\n+ * accepted values are\n+ *\n+ *\tto set true - \"1\", \"yes\", \"true\"\n+ *\tto set false - \"0\", \"no\", \"false\"\n+ *\n+ * This function returns 1 on success. It returns 0 when we don't find\n+ * an exact match for \"candidate\" or when the boolean value given is\n+ * not valid.\n+ */\n+static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, int *val)\n+{\n+\tconst char *argval;\n+\tchar *strval;\n+\tsize_t arglen;\n+\tint v;\n+\n+\tif (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))\n+\t\treturn 0;\n+\n+\tif (!argval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrval = xstrndup(argval, arglen);\n+\tv = git_parse_maybe_bool(strval);\n+\tfree(strval);\n+\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static int color_atom_parser(struct ref_format *format, struct used_atom *atom,\n \t\t\t const char *color_value, struct strbuf *err)\n {\n-- \n2.41.0.396.g9ab76b0018\n\n"},{"id":"479786","messageId":"20230723162717.68123-3-five231003@gmail.com","threadId":"59952","inReplyTo":"20230723162717.68123-1-five231003@gmail.com","subject":"[PATCH v4 2/2] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-23T16:19:59Z","receivedAt":"2023-07-23T16:28:44Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Duplicate the logic of %(describe) and friends from pretty to\nref-filter. In the future, this change helps in unifying both the\nformats as ref-filter will be able to do everything that pretty is doing\nand we can have a single interface.\n\nThe new atom \"describe\" and its friends are equivalent to the existing\npretty formats with the same name.\n\nHelped-by: Junio C Hamano \nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n Documentation/git-for-each-ref.txt | 23 +++++\n ref-filter.c | 125 ++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 138 +++++++++++++++++++++++++++++\n 3 files changed, 286 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex d9588767a9..11b2bc3121 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -264,6 +264,29 @@ ahead-behind:::\n \tcommits ahead and behind, respectively, when comparing the output\n \tref to the `` specified in the format.\n \n+describe[:options]::\n+\tA human-readable name, like linkgit:git-describe[1];\n+\tempty string for undescribable commits. The `describe` string may\n+\tbe followed by a colon and one or more comma-separated options.\n++\n+--\n+tags=;;\n+\tInstead of only considering annotated tags, consider\n+\tlightweight tags as well; see the corresponding option in\n+\tlinkgit:git-describe[1] for details.\n+abbrev=;;\n+\tUse at least hexadecimal digits; see the corresponding\n+\toption in linkgit:git-describe[1] for details.\n+match=;;\n+\tOnly consider tags matching the given `glob(7)` pattern,\n+\texcluding the \"refs/tags/\" prefix; see the corresponding option\n+\tin linkgit:git-describe[1] for details.\n+exclude=;;\n+\tDo not consider tags matching the given `glob(7)` pattern,\n+\texcluding the \"refs/tags/\" prefix; see the corresponding option\n+\tin linkgit:git-describe[1] for details.\n+--\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8d5f85e0a7..df00f1628c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -5,6 +5,7 @@\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"object-name.h\"\n@@ -146,6 +147,7 @@ enum atom_type {\n \tATOM_TAGGERDATE,\n \tATOM_CREATOR,\n \tATOM_CREATORDATE,\n+\tATOM_DESCRIBE,\n \tATOM_SUBJECT,\n \tATOM_BODY,\n \tATOM_TRAILERS,\n@@ -220,6 +222,7 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tconst char **describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \treturn 0;\n }\n \n+static int describe_atom_option_parser(struct strvec *args, const char **arg,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tconst char *argval;\n+\tsize_t arglen = 0;\n+\tint optval = 0;\n+\n+\tif (match_atom_bool_arg(*arg, \"tags\", arg, &optval)) {\n+\t\tif (!optval)\n+\t\t\tstrvec_push(args, \"--no-tags\");\n+\t\telse\n+\t\t\tstrvec_push(args, \"--tags\");\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"abbrev\", arg, &argval, &arglen)) {\n+\t\tchar *endptr;\n+\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"argument expected for %s\"),\n+\t\t\t\t\t \"describe:abbrev\");\n+\t\tif (strtol(argval, &endptr, 10) < 0)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"positive value expected %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\t\tif (endptr - argval != arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"cannot fully parse %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\n+\t\tstrvec_pushf(args, \"--abbrev=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"match\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:match\");\n+\n+\t\tstrvec_pushf(args, \"--match=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"exclude\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:exclude\");\n+\n+\t\tstrvec_pushf(args, \"--exclude=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int describe_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\tstruct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\n+\tfor (;;) {\n+\t\tint found = 0;\n+\t\tconst char *bad_arg = arg;\n+\n+\t\tif (!arg || !*arg)\n+\t\t\tbreak;\n+\n+\t\tfound = describe_atom_option_parser(&args, &arg, err);\n+\t\tif (found < 0)\n+\t\t\treturn found;\n+\t\tif (!found)\n+\t\t\treturn err_bad_arg(err, \"describe\", bad_arg);\n+\t}\n+\tatom->u.describe_args = strvec_detach(&args);\n+\treturn 0;\n+}\n+\n static int raw_atom_parser(struct ref_format *format UNUSED,\n \t\t\t struct used_atom *atom,\n \t\t\t const char *arg, struct strbuf *err)\n@@ -802,6 +886,7 @@ static struct {\n \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n+\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n@@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n \t}\n }\n \n+static void grab_describe_values(struct atom_value *val, int deref,\n+\t\t\t\t struct object *obj)\n+{\n+\tstruct commit *commit = (struct commit *)obj;\n+\tint i;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tenum atom_type type = atom->atom_type;\n+\t\tconst char *name = atom->name;\n+\t\tstruct atom_value *v = &val[i];\n+\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tif (type != ATOM_DESCRIBE)\n+\t\t\tcontinue;\n+\n+\t\tif (!!deref != (*name == '*'))\n+\t\t\tcontinue;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_push(&cmd.args, \"describe\");\n+\t\tstrvec_pushv(&cmd.args, atom->u.describe_args);\n+\t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n+\t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n+\t\t\terror(_(\"failed to run 'describe'\"));\n+\t\t\tv->s = xstrdup(\"\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_rtrim(&out);\n+\t\tv->s = strbuf_detach(&out, NULL);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)\n {\n@@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_tag_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n@@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n \t\tgrab_signature(val, deref, obj);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 910bf1ea94..7116e008f4 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -597,6 +597,144 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n+test_expect_success 'setup for describe atom tests' '\n+\tgit init -b master describe-repo &&\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\ttest_commit --no-tag one &&\n+\t\tgit tag tagone &&\n+\n+\t\ttest_commit --no-tag two &&\n+\t\tgit tag -a -m \"tag two\" tagtwo\n+\t)\n+'\n+\n+test_expect_success 'describe atom vs git describe' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname)\" \\\n+\t\t\trefs/tags/ >obj &&\n+\t\twhile read hash\n+\t\tdo\n+\t\t\tif desc=$(git describe $hash)\n+\t\t\tthen\n+\t\t\t\t: >expect-contains-good\n+\t\t\telse\n+\t\t\t\t: >expect-contains-bad\n+\t\t\tfi &&\n+\t\t\techo \"$hash $desc\" || return 1\n+\t\tdone expect &&\n+\t\ttest_path_exists expect-contains-good &&\n+\t\ttest_path_exists expect-contains-bad &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname) %(describe)\" \\\n+\t\t\trefs/tags/ >actual 2>err &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'describe:tags vs describe --tags' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit describe --tags >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:tags)\" \\\n+\t\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# Case 1: We have commits between HEAD and the most\n+\t\t#\t recent tag reachable from it\n+\t\ttest_commit --no-tag file &&\n+\t\tgit describe --abbrev=14 >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Make sure the hash used is atleast 14 digits long\n+\t\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\t\ttest 15 -le $(wc -c expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest tagname = $(cat actual)\n+\t)\n+'\n+\n+test_expect_success 'describe:match=... vs describe --match ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag foo\" tag-foo &&\n+\t\tgit describe --match \"*-foo\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:match=\"*-foo\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag bar\" tag-bar &&\n+\t\tgit describe --exclude \"*-bar\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:exclude=\"*-bar\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'deref with describe atom' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\ttagname\n+\t\ttagname\n+\t\ttagname\n+\n+\t\ttagtwo\n+\t\tEOF\n+\t\tgit for-each-ref --format=\"%(*describe)\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'err on bad describe atom arg' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# The bad arg is the only arg passed to describe atom\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfatal: unrecognized %(describe) argument: baz\n+\t\tEOF\n+\t\t! git for-each-ref --format=\"%(describe:baz)\" \\\n+\t\t\trefs/heads/master 2>actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The bad arg is in the middle of the option string\n+\t\t# passed to the describe atom\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfatal: unrecognized %(describe) argument: qux=1,abbrev=14\n+\t\tEOF\n+\t\t! git for-each-ref \\\n+\t\t\t--format=\"%(describe:tags,qux=1,abbrev=14)\" \\\n+\t\t\tref/heads/master 2>actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n cat >expected <<\\EOF\n heads/main\n tags/main\n-- \n2.41.0.396.g9ab76b0018\n\n"},{"id":"479819","messageId":"xmqqfs5dql84.fsf@gitster.g","threadId":"59952","inReplyTo":"20230723162717.68123-3-five231003@gmail.com","subject":"Re: [PATCH v4 2/2] ref-filter: add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T17:21:15Z","receivedAt":"2023-07-24T17:21:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> +test_expect_success 'err on bad describe atom arg' '\n> +\t(\n> +\t\tcd describe-repo &&\n> +\n> +\t\t# The bad arg is the only arg passed to describe atom\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\tfatal: unrecognized %(describe) argument: baz\n> +\t\tEOF\n> +\t\t! git for-each-ref --format=\"%(describe:baz)\" \\\n> +\t\t\trefs/heads/master 2>actual &&\n> +\t\ttest_cmp expect actual &&\n\nInstead of \"! git something\", use of \"test_must_fail git something\" is\nrecommended. The former would pass upon a crashing \"git\" happily, but\nthe latter would complain if \"git\" segfaults.\n\n> +\t\t# The bad arg is in the middle of the option string\n> +\t\t# passed to the describe atom\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\tfatal: unrecognized %(describe) argument: qux=1,abbrev=14\n> +\t\tEOF\n> +\t\t! git for-each-ref \\\n> +\t\t\t--format=\"%(describe:tags,qux=1,abbrev=14)\" \\\n> +\t\t\tref/heads/master 2>actual &&\n\nDitto.\n\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nOther than that, both patches looked good to me. Thanks.\n"},{"id":"479820","messageId":"xmqqa5vlqktr.fsf@gitster.g","threadId":"59952","inReplyTo":"20230723162717.68123-2-five231003@gmail.com","subject":"Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T17:29:52Z","receivedAt":"2023-07-24T17:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> The functions\n>\n> \tmatch_placeholder_arg_value()\n> \tmatch_placeholder_bool_arg()\n>\n> were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\n> with explicit value, 2019-01-29) to parse multiple options in an\n> argument to --pretty. For example,\n>\n> \tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n>\n> will output all the trailers matching the key and seperates them by\n> a comma followed by a space per commit.\n>\n> Add similar functions,\n>\n> \tmatch_atom_arg_value()\n> \tmatch_atom_bool_arg()\n>\n> in ref-filter.\n\nWhat are their similarities, and in what way are they different? If\nthey are similar enough, is it reasonable to allow these two pairs\nof helpers to share code (the best case would be we can just call\nthe existing ones, possibly changing their names to more suitable\nones that fit their now-more-general-purpose nature better)?\n\n> There is no atom yet that can use these functions in ref-filter, but we\n> are going to add a new %(describe) atom in a subsequent commit where we\n> parse options like tags= or match= given to it.\n>\n> Helped-by: Junio C Hamano \n> Mentored-by: Christian Couder \n> Mentored-by: Hariom Verma \n> Signed-off-by: Kousik Sanagavarapu \n> ---\n> ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 105 insertions(+)\n\nAsking just out of curiousity, all patches from you seem to have\n\"Mentored-by\" naming your mentors, but how deeply involved are they\nin each patch you send out? Is it like you first ask them to review\nand only after addressing the issues their reviews raise, you are\nsending the polished patches to the list? Or are they not deeply\ninvolved in the code but offering suggestions on the design (I am\ncurious what their reactions were on your design decision to\nadd the two helper functions)?\n\n"},{"id":"479822","messageId":"ZL6_DlDIE8Hfl_T6@five231003","threadId":"59952","inReplyTo":"xmqqa5vlqktr.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-24T18:12:30Z","receivedAt":"2023-07-24T18:12:59Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Mon, Jul 24, 2023 at 10:29:52AM -0700, Junio C Hamano wrote:\n> Kousik Sanagavarapu writes:\n> \n> > The functions\n> >\n> > \tmatch_placeholder_arg_value()\n> > \tmatch_placeholder_bool_arg()\n> >\n> > were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\n> > with explicit value, 2019-01-29) to parse multiple options in an\n> > argument to --pretty. For example,\n> >\n> > \tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n> >\n> > will output all the trailers matching the key and seperates them by\n> > a comma followed by a space per commit.\n> >\n> > Add similar functions,\n> >\n> > \tmatch_atom_arg_value()\n> > \tmatch_atom_bool_arg()\n> >\n> > in ref-filter.\n> \n> What are their similarities, and in what way are they different? If\n> they are similar enough, is it reasonable to allow these two pairs\n> of helpers to share code (the best case would be we can just call\n> the existing ones, possibly changing their names to more suitable\n> ones that fit their now-more-general-purpose nature better)?\n\nWhat do you mean by \"share code\"?\n\nThey are similar in their functionality, that is parsing the option and\ngrabbing the value (if the option has a value, otherwise we do what we\ndid here). The difference is the way we do such a parsing.\n\nIn pretty, we directly skip_prefix() the placeholder. So we check for ')'\nto see if we have reached the end of \"to_parse\".\n\nIn ref-filter (the current patches), we deal directly with the options\n(\"arg\" here), that is we can't do a check for ')' to see if we have\nexhausted our option list. So we can't really use the same functions, but\nthere is the possiblity that we can modify them to be used here too.\n\nSo the difference is mainly just how we send \"to_parse\" and how we want\nit parsed.\n\n> > There is no atom yet that can use these functions in ref-filter, but we\n> > are going to add a new %(describe) atom in a subsequent commit where we\n> > parse options like tags= or match= given to it.\n> >\n> > Helped-by: Junio C Hamano \n> > Mentored-by: Christian Couder \n> > Mentored-by: Hariom Verma \n> > Signed-off-by: Kousik Sanagavarapu \n> > ---\n> > ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 105 insertions(+)\n> \n> Asking just out of curiousity, all patches from you seem to have\n> \"Mentored-by\" naming your mentors, but how deeply involved are they\n> in each patch you send out? Is it like you first ask them to review\n> and only after addressing the issues their reviews raise, you are\n> sending the polished patches to the list? Or are they not deeply\n> involved in the code but offering suggestions on the design\n\nBoth actually, the code and the design. I send them the commits which I\npush to my fork and they take a look on the code as well as the design\nand offer suggestions on how both can be improved or re-did.\n\n> (I am\n> curious what their reactions were on your design decision to\n> add the two helper functions)?\n\nThey suggested doing something similar to what you suggested above but\nit is kind of on hold (also because of how we changed the implementation\nof \"match_atom_arg_value()\"). Now that you bring it up, should this\npatch be reworked?\n\nThanks\n"},{"id":"479830","messageId":"xmqqmszlnixe.fsf@gitster.g","threadId":"59952","inReplyTo":"ZL6_DlDIE8Hfl_T6@five231003","subject":"Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-24T20:39:09Z","receivedAt":"2023-07-24T20:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> What do you mean by \"share code\"?\n>\n> They are similar in their functionality, that is parsing the option and\n> grabbing the value (if the option has a value, otherwise we do what we\n> did here). The difference is the way we do such a parsing.\n>\n> In pretty, we directly skip_prefix() the placeholder. So we check for ')'\n> to see if we have reached the end of \"to_parse\".\n>\n> In ref-filter (the current patches), we deal directly with the options\n> (\"arg\" here), that is we can't do a check for ')' to see if we have\n> exhausted our option list. So we can't really use the same functions, but\n> there is the possiblity that we can modify them to be used here too.\n\nThat is the kind of \"sharing\" to reduce repetition I had in mind.\n\nI haven't checked the callers, but another way would be to update\nthe caller of for-each-ref's side to match the calling convention of\nhow pretty calls the parser, wouldn't it? After all, they parse the\nsame \"%(token:key=val,key=val,...)\" so...?\n"},{"id":"479849","messageId":"xmqqv8e7lrkd.fsf@gitster.g","threadId":"59952","inReplyTo":"xmqqmszlnixe.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-25T19:27:46Z","receivedAt":"2023-07-25T19:27:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Kousik Sanagavarapu writes:\n>\n>> What do you mean by \"share code\"?\n>>\n>> They are similar in their functionality, that is parsing the option and\n>> grabbing the value (if the option has a value, otherwise we do what we\n>> did here). The difference is the way we do such a parsing.\n>>\n>> In pretty, we directly skip_prefix() the placeholder. So we check for ')'\n>> to see if we have reached the end of \"to_parse\".\n>>\n>> In ref-filter (the current patches), we deal directly with the options\n>> (\"arg\" here), that is we can't do a check for ')' to see if we have\n>> exhausted our option list. So we can't really use the same functions, but\n>> there is the possiblity that we can modify them to be used here too.\n>\n> That is the kind of \"sharing\" to reduce repetition I had in mind.\n>\n> I haven't checked the callers, but another way would be to update\n> the caller of for-each-ref's side to match the calling convention of\n> how pretty calls the parser, wouldn't it? After all, they parse the\n> same \"%(token:key=val,key=val,...)\" so...?\n\nHaving said all that, let's move things forward by merging this\nversion. Anybody (it could be you) interested in cleaning up by\nunifying these two very similar implementations of the same thing\ncan do so on top.\n\nThanks.\n"},{"id":"479856","messageId":"20230725205924.40585-1-five231003@gmail.com","threadId":"59952","inReplyTo":"20230723162717.68123-1-five231003@gmail.com","subject":"[PATCH v5 0/2] Add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-25T20:51:20Z","receivedAt":"2023-07-25T21:00:26Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Hi,\nThanks for the review on the previous version of this series. Here is a\nquick re-roll to address the minor changes that you left on the previous\nversion (apart from the suggestions to PATCH 1/2).\n\nPlease queue this instead.\n\nPATCH 1/2 - Left unchanged\n\nPATCH 2/2 - Used the test helper function `test_must_fail` instead of\n\t something like \"! git foo\" for making a command fail.\n\nKousik Sanagavarapu (2):\n ref-filter: add multiple-option parsing functions\n ref-filter: add new \"describe\" atom\n\n Documentation/git-for-each-ref.txt | 23 +++\n ref-filter.c | 230 +++++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 139 +++++++++++++++++\n 3 files changed, 392 insertions(+)\n\nRange-diff against v4:\n1: 2914bd58ec = 1: 2914bd58ec ref-filter: add multiple-option parsing functions\n2: 77a2a56520 ! 2: 8127f4399c ref-filter: add new \"describe\" atom\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n + cat >expect <<-\\EOF &&\n + fatal: unrecognized %(describe) argument: baz\n + EOF\n -+ ! git for-each-ref --format=\"%(describe:baz)\" \\\n ++ test_must_fail git for-each-ref \\\n ++ --format=\"%(describe:baz)\" \\\n + refs/heads/master 2>actual &&\n + test_cmp expect actual &&\n +\n @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override\n + cat >expect <<-\\EOF &&\n + fatal: unrecognized %(describe) argument: qux=1,abbrev=14\n + EOF\n -+ ! git for-each-ref \\\n ++ test_must_fail git for-each-ref \\\n + --format=\"%(describe:tags,qux=1,abbrev=14)\" \\\n + ref/heads/master 2>actual &&\n + test_cmp expect actual\n\n-- \n2.41.0.396.g77a2a56520\n\n"},{"id":"479857","messageId":"20230725205924.40585-2-five231003@gmail.com","threadId":"59952","inReplyTo":"20230725205924.40585-1-five231003@gmail.com","subject":"[PATCH v5 1/2] ref-filter: add multiple-option parsing functions","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-25T20:51:21Z","receivedAt":"2023-07-25T21:00:43Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"The functions\n\n\tmatch_placeholder_arg_value()\n\tmatch_placeholder_bool_arg()\n\nwere added in pretty 4f732e0fd7 (pretty: allow %(trailers) options\nwith explicit value, 2019-01-29) to parse multiple options in an\nargument to --pretty. For example,\n\n\tgit log --pretty=\"%(trailers:key=Signed-Off-By,separator=%x2C )\"\n\nwill output all the trailers matching the key and seperates them by\na comma followed by a space per commit.\n\nAdd similar functions,\n\n\tmatch_atom_arg_value()\n\tmatch_atom_bool_arg()\n\nin ref-filter.\n\nThere is no atom yet that can use these functions in ref-filter, but we\nare going to add a new %(describe) atom in a subsequent commit where we\nparse options like tags= or match= given to it.\n\nHelped-by: Junio C Hamano \nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 105 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0f3df132b8..8d5f85e0a7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1,6 +1,7 @@\n #include \"git-compat-util.h\"\n #include \"environment.h\"\n #include \"gettext.h\"\n+#include \"config.h\"\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n@@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)\n \treturn -1;\n }\n \n+/*\n+ * Parse option of name \"candidate\" in the option string \"to_parse\" of\n+ * the form\n+ *\n+ *\t\"candidate1[=val1],candidate2[=val2],candidate3[=val3],...\"\n+ *\n+ * The remaining part of \"to_parse\" is stored in \"end\" (if we are\n+ * parsing the last candidate, then this is NULL) and the value of\n+ * the candidate is stored in \"valuestart\" and its length in \"valuelen\",\n+ * that is the portion after \"=\". Since it is possible for a \"candidate\"\n+ * to not have a value, in such cases, \"valuestart\" is set to point to\n+ * NULL and \"valuelen\" to 0.\n+ *\n+ * The function returns 1 on success. It returns 0 if we don't find\n+ * \"candidate\" in \"to_parse\" or we find \"candidate\" but it is followed\n+ * by more chars (for example, \"candidatefoo\"), that is, we don't find\n+ * an exact match.\n+ *\n+ * This function only does the above for one \"candidate\" at a time. So\n+ * it has to be called each time trying to parse a \"candidate\" in the\n+ * option string \"to_parse\".\n+ */\n+static int match_atom_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, const char **valuestart,\n+\t\t\t\tsize_t *valuelen)\n+{\n+\tconst char *atom;\n+\n+\tif (!skip_prefix(to_parse, candidate, &atom))\n+\t\treturn 0; /* definitely not \"candidate\" */\n+\n+\tif (*atom == '=') {\n+\t\t/* we just saw \"candidate=\" */\n+\t\t*valuestart = atom + 1;\n+\t\tatom = strchrnul(*valuestart, ',');\n+\t\t*valuelen = atom - *valuestart;\n+\t} else if (*atom != ',' && *atom != '\\0') {\n+\t\t/* key begins with \"candidate\" but has more chars */\n+\t\treturn 0;\n+\t} else {\n+\t\t/* just \"candidate\" without \"=val\" */\n+\t\t*valuestart = NULL;\n+\t\t*valuelen = 0;\n+\t}\n+\n+\t/* atom points at either the ',' or NUL after this key[=val] */\n+\tif (*atom == ',')\n+\t\tatom++;\n+\telse if (*atom)\n+\t\tBUG(\"Why is *atom not NULL yet?\");\n+\n+\t*end = atom;\n+\treturn 1;\n+}\n+\n+/*\n+ * Parse boolean option of name \"candidate\" in the option list \"to_parse\"\n+ * of the form\n+ *\n+ *\t\"candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],...\"\n+ *\n+ * The remaining part of \"to_parse\" is stored in \"end\" (if we are parsing\n+ * the last candidate, then this is NULL) and the value (if given) is\n+ * parsed and stored in \"val\", so \"val\" always points to either 0 or 1.\n+ * If the value is not given, then \"val\" is set to point to 1.\n+ *\n+ * The boolean value is parsed using \"git_parse_maybe_bool()\", so the\n+ * accepted values are\n+ *\n+ *\tto set true - \"1\", \"yes\", \"true\"\n+ *\tto set false - \"0\", \"no\", \"false\"\n+ *\n+ * This function returns 1 on success. It returns 0 when we don't find\n+ * an exact match for \"candidate\" or when the boolean value given is\n+ * not valid.\n+ */\n+static int match_atom_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\tconst char **end, int *val)\n+{\n+\tconst char *argval;\n+\tchar *strval;\n+\tsize_t arglen;\n+\tint v;\n+\n+\tif (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen))\n+\t\treturn 0;\n+\n+\tif (!argval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrval = xstrndup(argval, arglen);\n+\tv = git_parse_maybe_bool(strval);\n+\tfree(strval);\n+\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static int color_atom_parser(struct ref_format *format, struct used_atom *atom,\n \t\t\t const char *color_value, struct strbuf *err)\n {\n-- \n2.41.0.396.g77a2a56520\n\n"},{"id":"479858","messageId":"20230725205924.40585-3-five231003@gmail.com","threadId":"59952","inReplyTo":"20230725205924.40585-1-five231003@gmail.com","subject":"[PATCH v5 2/2] ref-filter: add new \"describe\" atom","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-07-25T20:51:22Z","receivedAt":"2023-07-25T21:00:56Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Duplicate the logic of %(describe) and friends from pretty to\nref-filter. In the future, this change helps in unifying both the\nformats as ref-filter will be able to do everything that pretty is doing\nand we can have a single interface.\n\nThe new atom \"describe\" and its friends are equivalent to the existing\npretty formats with the same name.\n\nHelped-by: Junio C Hamano \nMentored-by: Christian Couder \nMentored-by: Hariom Verma \nSigned-off-by: Kousik Sanagavarapu \n---\n Documentation/git-for-each-ref.txt | 23 +++++\n ref-filter.c | 125 ++++++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 139 +++++++++++++++++++++++++++++\n 3 files changed, 287 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex d9588767a9..11b2bc3121 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -264,6 +264,29 @@ ahead-behind:::\n \tcommits ahead and behind, respectively, when comparing the output\n \tref to the `` specified in the format.\n \n+describe[:options]::\n+\tA human-readable name, like linkgit:git-describe[1];\n+\tempty string for undescribable commits. The `describe` string may\n+\tbe followed by a colon and one or more comma-separated options.\n++\n+--\n+tags=;;\n+\tInstead of only considering annotated tags, consider\n+\tlightweight tags as well; see the corresponding option in\n+\tlinkgit:git-describe[1] for details.\n+abbrev=;;\n+\tUse at least hexadecimal digits; see the corresponding\n+\toption in linkgit:git-describe[1] for details.\n+match=;;\n+\tOnly consider tags matching the given `glob(7)` pattern,\n+\texcluding the \"refs/tags/\" prefix; see the corresponding option\n+\tin linkgit:git-describe[1] for details.\n+exclude=;;\n+\tDo not consider tags matching the given `glob(7)` pattern,\n+\texcluding the \"refs/tags/\" prefix; see the corresponding option\n+\tin linkgit:git-describe[1] for details.\n+--\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8d5f85e0a7..df00f1628c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -5,6 +5,7 @@\n #include \"gpg-interface.h\"\n #include \"hex.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n #include \"refs.h\"\n #include \"wildmatch.h\"\n #include \"object-name.h\"\n@@ -146,6 +147,7 @@ enum atom_type {\n \tATOM_TAGGERDATE,\n \tATOM_CREATOR,\n \tATOM_CREATORDATE,\n+\tATOM_DESCRIBE,\n \tATOM_SUBJECT,\n \tATOM_BODY,\n \tATOM_TRAILERS,\n@@ -220,6 +222,7 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tconst char **describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \treturn 0;\n }\n \n+static int describe_atom_option_parser(struct strvec *args, const char **arg,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tconst char *argval;\n+\tsize_t arglen = 0;\n+\tint optval = 0;\n+\n+\tif (match_atom_bool_arg(*arg, \"tags\", arg, &optval)) {\n+\t\tif (!optval)\n+\t\t\tstrvec_push(args, \"--no-tags\");\n+\t\telse\n+\t\t\tstrvec_push(args, \"--tags\");\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"abbrev\", arg, &argval, &arglen)) {\n+\t\tchar *endptr;\n+\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"argument expected for %s\"),\n+\t\t\t\t\t \"describe:abbrev\");\n+\t\tif (strtol(argval, &endptr, 10) < 0)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"positive value expected %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\t\tif (endptr - argval != arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"cannot fully parse %s=%s\"),\n+\t\t\t\t\t \"describe:abbrev\", argval);\n+\n+\t\tstrvec_pushf(args, \"--abbrev=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"match\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:match\");\n+\n+\t\tstrvec_pushf(args, \"--match=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_atom_arg_value(*arg, \"exclude\", arg, &argval, &arglen)) {\n+\t\tif (!arglen)\n+\t\t\treturn strbuf_addf_ret(err, -1,\n+\t\t\t\t\t _(\"value expected %s=\"),\n+\t\t\t\t\t \"describe:exclude\");\n+\n+\t\tstrvec_pushf(args, \"--exclude=%.*s\", (int)arglen, argval);\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int describe_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\tstruct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\n+\tfor (;;) {\n+\t\tint found = 0;\n+\t\tconst char *bad_arg = arg;\n+\n+\t\tif (!arg || !*arg)\n+\t\t\tbreak;\n+\n+\t\tfound = describe_atom_option_parser(&args, &arg, err);\n+\t\tif (found < 0)\n+\t\t\treturn found;\n+\t\tif (!found)\n+\t\t\treturn err_bad_arg(err, \"describe\", bad_arg);\n+\t}\n+\tatom->u.describe_args = strvec_detach(&args);\n+\treturn 0;\n+}\n+\n static int raw_atom_parser(struct ref_format *format UNUSED,\n \t\t\t struct used_atom *atom,\n \t\t\t const char *arg, struct strbuf *err)\n@@ -802,6 +886,7 @@ static struct {\n \t[ATOM_TAGGERDATE] = { \"taggerdate\", SOURCE_OBJ, FIELD_TIME },\n \t[ATOM_CREATOR] = { \"creator\", SOURCE_OBJ },\n \t[ATOM_CREATORDATE] = { \"creatordate\", SOURCE_OBJ, FIELD_TIME },\n+\t[ATOM_DESCRIBE] = { \"describe\", SOURCE_OBJ, FIELD_STR, describe_atom_parser },\n \t[ATOM_SUBJECT] = { \"subject\", SOURCE_OBJ, FIELD_STR, subject_atom_parser },\n \t[ATOM_BODY] = { \"body\", SOURCE_OBJ, FIELD_STR, body_atom_parser },\n \t[ATOM_TRAILERS] = { \"trailers\", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },\n@@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n \t}\n }\n \n+static void grab_describe_values(struct atom_value *val, int deref,\n+\t\t\t\t struct object *obj)\n+{\n+\tstruct commit *commit = (struct commit *)obj;\n+\tint i;\n+\n+\tfor (i = 0; i < used_atom_cnt; i++) {\n+\t\tstruct used_atom *atom = &used_atom[i];\n+\t\tenum atom_type type = atom->atom_type;\n+\t\tconst char *name = atom->name;\n+\t\tstruct atom_value *v = &val[i];\n+\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tif (type != ATOM_DESCRIBE)\n+\t\t\tcontinue;\n+\n+\t\tif (!!deref != (*name == '*'))\n+\t\t\tcontinue;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_push(&cmd.args, \"describe\");\n+\t\tstrvec_pushv(&cmd.args, atom->u.describe_args);\n+\t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n+\t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n+\t\t\terror(_(\"failed to run 'describe'\"));\n+\t\t\tv->s = xstrdup(\"\");\n+\t\t\tcontinue;\n+\t\t}\n+\t\tstrbuf_rtrim(&out);\n+\t\tv->s = strbuf_detach(&out, NULL);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+}\n+\n /* See grab_values */\n static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data)\n {\n@@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_tag_values(val, deref, obj);\n \t\tgrab_sub_body_contents(val, deref, data);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n@@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n \t\tgrab_signature(val, deref, obj);\n+\t\tgrab_describe_values(val, deref, obj);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 910bf1ea94..16082dccb7 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -597,6 +597,145 @@ test_expect_success 'color.ui=always does not override tty check' '\n \ttest_cmp expected.bare actual\n '\n \n+test_expect_success 'setup for describe atom tests' '\n+\tgit init -b master describe-repo &&\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\ttest_commit --no-tag one &&\n+\t\tgit tag tagone &&\n+\n+\t\ttest_commit --no-tag two &&\n+\t\tgit tag -a -m \"tag two\" tagtwo\n+\t)\n+'\n+\n+test_expect_success 'describe atom vs git describe' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname)\" \\\n+\t\t\trefs/tags/ >obj &&\n+\t\twhile read hash\n+\t\tdo\n+\t\t\tif desc=$(git describe $hash)\n+\t\t\tthen\n+\t\t\t\t: >expect-contains-good\n+\t\t\telse\n+\t\t\t\t: >expect-contains-bad\n+\t\t\tfi &&\n+\t\t\techo \"$hash $desc\" || return 1\n+\t\tdone expect &&\n+\t\ttest_path_exists expect-contains-good &&\n+\t\ttest_path_exists expect-contains-bad &&\n+\n+\t\tgit for-each-ref --format=\"%(objectname) %(describe)\" \\\n+\t\t\trefs/tags/ >actual 2>err &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n+test_expect_success 'describe:tags vs describe --tags' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit describe --tags >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:tags)\" \\\n+\t\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:abbrev=... vs describe --abbrev=...' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# Case 1: We have commits between HEAD and the most\n+\t\t#\t recent tag reachable from it\n+\t\ttest_commit --no-tag file &&\n+\t\tgit describe --abbrev=14 >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Make sure the hash used is atleast 14 digits long\n+\t\tsed -e \"s/^.*-g\\([0-9a-f]*\\)$/\\1/\" hexpart &&\n+\t\ttest 15 -le $(wc -c expect &&\n+\t\tgit for-each-ref --format=\"%(describe:abbrev=14)\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest tagname = $(cat actual)\n+\t)\n+'\n+\n+test_expect_success 'describe:match=... vs describe --match ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag foo\" tag-foo &&\n+\t\tgit describe --match \"*-foo\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:match=\"*-foo\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'describe:exclude:... vs describe --exclude ...' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tgit tag -a -m \"tag bar\" tag-bar &&\n+\t\tgit describe --exclude \"*-bar\" >expect &&\n+\t\tgit for-each-ref --format=\"%(describe:exclude=\"*-bar\")\" \\\n+\t\t\trefs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'deref with describe atom' '\n+\t(\n+\t\tcd describe-repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\n+\t\ttagname\n+\t\ttagname\n+\t\ttagname\n+\n+\t\ttagtwo\n+\t\tEOF\n+\t\tgit for-each-ref --format=\"%(*describe)\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'err on bad describe atom arg' '\n+\t(\n+\t\tcd describe-repo &&\n+\n+\t\t# The bad arg is the only arg passed to describe atom\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfatal: unrecognized %(describe) argument: baz\n+\t\tEOF\n+\t\ttest_must_fail git for-each-ref \\\n+\t\t\t--format=\"%(describe:baz)\" \\\n+\t\t\trefs/heads/master 2>actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# The bad arg is in the middle of the option string\n+\t\t# passed to the describe atom\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfatal: unrecognized %(describe) argument: qux=1,abbrev=14\n+\t\tEOF\n+\t\ttest_must_fail git for-each-ref \\\n+\t\t\t--format=\"%(describe:tags,qux=1,abbrev=14)\" \\\n+\t\t\tref/heads/master 2>actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n cat >expected <<\\EOF\n heads/main\n tags/main\n-- \n2.41.0.396.g77a2a56520\n\n"},{"id":"479864","messageId":"xmqq4jlrirzq.fsf@gitster.g","threadId":"59952","inReplyTo":"20230725205924.40585-1-five231003@gmail.com","subject":"Re: [PATCH v5 0/2] Add new \"describe\" atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-25T21:46:49Z","receivedAt":"2023-07-25T21:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> Thanks for the review on the previous version of this series. Here is a\n> quick re-roll to address the minor changes that you left on the previous\n> version (apart from the suggestions to PATCH 1/2).\n>\n> Please queue this instead.\n\n\"! -> test_must_fail\" has already been corrected locally yesterday\nbefore I pushed the integration results out, so I'd skip this round.\n"}]}