* [PATCH 0/2] Add new "describe" atom
@ 2023-07-05 17:57 Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
` (2 more replies)
0 siblings, 3 replies; 38+ messages in thread
From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw)
To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu
Hi,
This patch series focuses on duplicating the implementation of
%(describe) and its friends from pretty to ref-filter, with the end goal
of making ref-filter do everything that pretty is doing.
PATCH 1/2 - This patch is the duplication of the placeholder from pretty
ref-filter.
PATCH 2/2 - This is an interesting case, where the tests written for the
above duplication are successful but another test below, in
t6300, "Verify sorts with raw:size" fails on linux-sha256 (CI).
Kousik Sanagavarapu (2):
ref-filter: add new "describe" atom
t6300: run describe atom tests on a different repo
Documentation/git-for-each-ref.txt | 19 ++++
ref-filter.c | 144 +++++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 98 ++++++++++++++++++++
3 files changed, 261 insertions(+)
--
2.41.0.237.g2d10a112d6.dirty
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 1/2] ref-filter: add new "describe" atom 2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu @ 2023-07-05 17:57 ` Kousik Sanagavarapu 2023-07-06 16:58 ` Junio C Hamano 2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu 2 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw) To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu, Christian Couder, Hariom Verma Duplicate the logic of %(describe) and friends from pretty to ref-filter. In the future, this change helps in unifying both the formats as ref-filter will be able to do everything that pretty is doing and we can have a single interface. The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/git-for-each-ref.txt | 19 ++++ ref-filter.c | 144 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 85 +++++++++++++++++ 3 files changed, 248 insertions(+) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1e215d4e73..4ac5c3dac4 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -231,6 +231,25 @@ ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. +describe[:options]:: human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ +** tags=<bool-value>: Instead of only considering annotated tags, consider + lightweight tags as well. +** abbrev=<number>: Instead of using the default number of hexadecimal digits + (which will vary according to the number of objects in the + repository with a default of 7) of the abbreviated + object name, use <number> digits, or as many digits as + needed to form a unique object name. +** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix. +** exclude=<pattern>: Do not consider tags matching the given `glob(7)` + pattern,excluding the "refs/tags/" prefix. + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/ref-filter.c b/ref-filter.c index e0d03a9f8e..6ec647c81f 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -5,6 +5,7 @@ #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" +#include "run-command.h" #include "refs.h" #include "wildmatch.h" #include "object-name.h" @@ -146,6 +147,7 @@ enum atom_type { ATOM_TAGGERDATE, ATOM_CREATOR, ATOM_CREATORDATE, + ATOM_DESCRIBE, ATOM_SUBJECT, ATOM_BODY, ATOM_TRAILERS, @@ -215,6 +217,13 @@ static struct used_atom { struct email_option { enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; } email_option; + struct { + enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE, + D_MATCH } option; + unsigned int tagbool; + unsigned int length; + char *pattern; + } describe; struct refname_atom refname; char *head; } u; @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato return 0; } +static int parse_describe_option(const char *arg) +{ + if (!arg) + return D_BARE; + else if (starts_with(arg, "tags")) + return D_TAGS; + else if (starts_with(arg, "abbrev")) + return D_ABBREV; + else if(starts_with(arg, "exclude")) + return D_EXCLUDE; + else if (starts_with(arg, "match")) + return D_MATCH; + return -1; +} + +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + int opt = parse_describe_option(arg); + + switch (opt) { + case D_BARE: + break; + case D_TAGS: + /* + * It is also possible to just use describe:tags, which + * is just treated as describe:tags=1 + */ + if (skip_prefix(arg, "tags=", &arg)) { + if (strtoul_ui(arg, 10, &atom->u.describe.tagbool)) + return strbuf_addf_ret(err, -1, _("boolean value " + "expected describe:tags=%s"), arg); + + } else { + atom->u.describe.tagbool = 1; + } + break; + case D_ABBREV: + skip_prefix(arg, "abbrev=", &arg); + if (strtoul_ui(arg, 10, &atom->u.describe.length)) + return strbuf_addf_ret(err, -1, _("positive value " + "expected describe:abbrev=%s"), arg); + break; + case D_EXCLUDE: + skip_prefix(arg, "exclude=", &arg); + atom->u.describe.pattern = xstrdup(arg); + break; + case D_MATCH: + skip_prefix(arg, "match=", &arg); + atom->u.describe.pattern = xstrdup(arg); + break; + default: + return err_bad_arg(err, "describe", arg); + break; + } + atom->u.describe.option = opt; + return 0; +} + static int raw_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) @@ -664,6 +733,7 @@ static struct { [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } } +static void grab_describe_values(struct atom_value *val, int deref, + struct object *obj) +{ + struct commit *commit = (struct commit *)obj; + int i; + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + int opt; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + + if (!!deref != (*name == '*')) + continue; + if (deref) + name++; + + if (!skip_prefix(name, "describe", &name) || + (*name && *name != ':')) + continue; + if (!*name) + name = NULL; + else + name++; + + opt = parse_describe_option(name); + if (opt < 0) + continue; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); + + switch(opt) { + case D_BARE: + break; + case D_TAGS: + if (atom->u.describe.tagbool) + strvec_push(&cmd.args, "--tags"); + else + strvec_push(&cmd.args, "--no-tags"); + break; + case D_ABBREV: + strvec_pushf(&cmd.args, "--abbrev=%d", + atom->u.describe.length); + break; + case D_EXCLUDE: + strvec_pushf(&cmd.args, "--exclude=%s", + atom->u.describe.pattern); + break; + case D_MATCH: + strvec_pushf(&cmd.args, "--match=%s", + atom->u.describe.pattern); + break; + } + + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); + v->s = xstrdup(""); + continue; + } + strbuf_rtrim(&out); + v->s = strbuf_detach(&out, NULL); + + strbuf_release(&err); + } +} + /* See grab_values */ static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data) { @@ -1592,12 +1734,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_tag_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("tagger", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: /* grab_tree_values(val, deref, obj, buf, sz); */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 5c00607608..98ea37d336 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' +test_expect_success 'describe atom vs git describe' ' + test_when_finished "rm -rf describe-repo" && + + git init describe-repo && + ( + cd describe-repo && + + test_commit --no-tag one && + git tag tagone && + + test_commit --no-tag two && + git tag -a -m "tag two" tagtwo && + + git for-each-ref refs/tags/ --format="%(objectname)" >obj && + while read hash + do + if desc=$(git describe $hash) + then + : >expect-contains-good + else + : >expect-contains-bad + fi && + echo "$hash $desc" || return 1 + done <obj >expect && + test_path_exists expect-contains-good && + test_path_exists expect-contains-bad && + + git for-each-ref --format="%(objectname) %(describe)" \ + refs/tags/ >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + ) +' + +test_expect_success 'describe:tags vs describe --tags' ' + test_when_finished "git tag -d tagname" && + git tag tagname && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" refs/heads/ >actual && + test_cmp expect actual +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' + test_when_finished "git tag -d tagname" && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + test tagname = $(cat actual) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' + test_when_finished "git tag -d tag-match" && + git tag -a -m "tag match" tag-match && + git describe --match "*-match" >expect && + git for-each-ref --format="%(describe:match="*-match")" \ + refs/heads/ >actual && + test_cmp expect actual +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' + test_when_finished "git tag -d tag-exclude" && + git tag -a -m "tag exclude" tag-exclude && + git describe --exclude "*-exclude" >expect && + git for-each-ref --format="%(describe:exclude="*-exclude")" \ + refs/heads/ >actual && + test_cmp expect actual +' + cat >expected <<\EOF heads/main tags/main -- 2.41.0.237.g2d10a112d6.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] ref-filter: add new "describe" atom 2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu @ 2023-07-06 16:58 ` Junio C Hamano 2023-07-09 6:16 ` Kousik Sanagavarapu 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-06 16:58 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Rene Scharfe, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > +describe[:options]:: human-readable name, like > + link-git:git-describe[1]; empty string for > + undescribable commits. The `describe` string may be > + followed by a colon and zero or more comma-separated > + options. Descriptions can be inconsistent when tags > + are added or removed at the same time. > ++ > +** tags=<bool-value>: Instead of only considering annotated tags, consider > + lightweight tags as well. > +** abbrev=<number>: Instead of using the default number of hexadecimal digits > + (which will vary according to the number of objects in the > + repository with a default of 7) of the abbreviated > + object name, use <number> digits, or as many digits as > + needed to form a unique object name. > +** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, > + excluding the "refs/tags/" prefix. > +** exclude=<pattern>: Do not consider tags matching the given `glob(7)` > + pattern,excluding the "refs/tags/" prefix. You are missing a SP after the comma in "pattern,excluding" above. The above description is slightly different from what "git describe --help" has. If they are described differently on purpose (e.g. you may have made "%(describe:abbrev=0)" not to show only the closest tag, unlike "git describe --abbrev=0"), the differences should be spelled out more explicitly. If the behaviours of the option here and the corresponding one there are meant to be the same, then either using exactly the same text, or a much abbreviated description with a note referring to the option description of "git describe", would help the readers better. E.g. abbrev=<number>;; use at least <number> hexadecimal digits; see the corresponding option in linkgit:git-describe[1] for details. which would make it clear that no behavioral differences are meant. This new section becomes a part of an existing "labeled list" (asciidoctor calls the construct "description list"). Starting the new heading this patch adds with 'describe[:options]::' makes sense. It is in line with the existing text. I however think that the list of options is better done as a nested description list. Documentation/config/color.txt has an example you can imitate. See how slots of color.grep.<slot> are described there. https://docs.asciidoctor.org/asciidoc/latest/lists/description/ https://asciidoc-py.github.io/userguide.html#_labeled_lists > diff --git a/ref-filter.c b/ref-filter.c > index e0d03a9f8e..6ec647c81f 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -5,6 +5,7 @@ > #include "gpg-interface.h" > #include "hex.h" > #include "parse-options.h" > +#include "run-command.h" > #include "refs.h" > #include "wildmatch.h" > #include "object-name.h" > @@ -146,6 +147,7 @@ enum atom_type { > ATOM_TAGGERDATE, > ATOM_CREATOR, > ATOM_CREATORDATE, > + ATOM_DESCRIBE, > ATOM_SUBJECT, > ATOM_BODY, > ATOM_TRAILERS, > @@ -215,6 +217,13 @@ static struct used_atom { > struct email_option { > enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; > } email_option; > + struct { > + enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE, > + D_MATCH } option; > + unsigned int tagbool; > + unsigned int length; The name "tagbool" sounds strange, as we are not saying "lengthint". > + char *pattern; > + } describe; I am a bit confused by this structure, actually, as I cannot quite guess from the data structure alone how you intend to use it. Does this give a good representation for the piece of data you are trying to capture? For example, %(describe:tags=no,abbrev=4) would become a single atom with 0 in .tagbool and 4 in .length, but what does the .option member get? > @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato > return 0; > } > > +static int parse_describe_option(const char *arg) > +{ > + if (!arg) > + return D_BARE; > + else if (starts_with(arg, "tags")) > + return D_TAGS; > + else if (starts_with(arg, "abbrev")) > + return D_ABBREV; > + else if(starts_with(arg, "exclude")) > + return D_EXCLUDE; > + else if (starts_with(arg, "match")) > + return D_MATCH; > + return -1; > +} > + > +static int describe_atom_parser(struct ref_format *format UNUSED, > + struct used_atom *atom, > + const char *arg, struct strbuf *err) > +{ > + int opt = parse_describe_option(arg); > + > + switch (opt) { > + case D_BARE: > + break; > + case D_TAGS: > + /* > + * It is also possible to just use describe:tags, which > + * is just treated as describe:tags=1 > + */ > + if (skip_prefix(arg, "tags=", &arg)) { > + if (strtoul_ui(arg, 10, &atom->u.describe.tagbool)) This is not how you accept a Boolean. "1", "0", "yes", "no", "true", "false", "on", "off" are all valid values and you use git_parse_maybe_bool() to parse them. > + return strbuf_addf_ret(err, -1, _("boolean value " > + "expected describe:tags=%s"), arg); > + > + } else { > + atom->u.describe.tagbool = 1; > + } > + break; > + case D_ABBREV: > + skip_prefix(arg, "abbrev=", &arg); > + if (strtoul_ui(arg, 10, &atom->u.describe.length)) > + return strbuf_addf_ret(err, -1, _("positive value " > + "expected describe:abbrev=%s"), arg); > + break; > + case D_EXCLUDE: > + skip_prefix(arg, "exclude=", &arg); > + atom->u.describe.pattern = xstrdup(arg); > + break; > + case D_MATCH: > + skip_prefix(arg, "match=", &arg); > + atom->u.describe.pattern = xstrdup(arg); > + break; > + default: > + return err_bad_arg(err, "describe", arg); > + break; > + } > + atom->u.describe.option = opt; > + return 0; > +} Even though the documentation patch we saw earlier said "may be followed by a colon and zero or more comma-separated options", this seems to expect only and exactly one option. Indeed, if we run the resulting git like "git for-each-ref --format='%(describe:tags=0,abbrev=4)'" you will get complaints from this parser. The implementation needs redesigning as the data structure is not equipped to handle more than one options given at the same time, as we saw earlier. > @@ -664,6 +733,7 @@ static struct { > [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, > [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, > [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, > + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, > [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, > [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, > [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, > @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size > } > } > > +static void grab_describe_values(struct atom_value *val, int deref, > + struct object *obj) > +{ > + struct commit *commit = (struct commit *)obj; > + int i; > + > + for (i = 0; i < used_atom_cnt; i++) { > + struct used_atom *atom = &used_atom[i]; > + const char *name = atom->name; > + struct atom_value *v = &val[i]; > + int opt; > + > + struct child_process cmd = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct strbuf err = STRBUF_INIT; > + > + if (!!deref != (*name == '*')) > + continue; > + if (deref) > + name++; > + > + if (!skip_prefix(name, "describe", &name) || > + (*name && *name != ':')) > + continue; This looks overly expensive. Why aren't we looking at the atom_type and see if it is ATOM_DESCRIBE here? > + switch(opt) { > + case D_BARE: > + break; > + case D_TAGS: > + if (atom->u.describe.tagbool) > + strvec_push(&cmd.args, "--tags"); > + else > + strvec_push(&cmd.args, "--no-tags"); > + break; > + case D_ABBREV: > + strvec_pushf(&cmd.args, "--abbrev=%d", > + atom->u.describe.length); > + break; > + case D_EXCLUDE: > + strvec_pushf(&cmd.args, "--exclude=%s", > + atom->u.describe.pattern); > + break; > + case D_MATCH: > + strvec_pushf(&cmd.args, "--match=%s", > + atom->u.describe.pattern); > + break; > + } Again, it is apparent here that the atom takes only one option at most. I'll stop here. Thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] ref-filter: add new "describe" atom 2023-07-06 16:58 ` Junio C Hamano @ 2023-07-09 6:16 ` Kousik Sanagavarapu 0 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-09 6:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma On Thu, Jul 06, 2023 at 09:58:09AM -0700, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > +describe[:options]:: human-readable name, like > > + link-git:git-describe[1]; empty string for > > + undescribable commits. The `describe` string may be > > + followed by a colon and zero or more comma-separated > > + options. Descriptions can be inconsistent when tags > > + are added or removed at the same time. > > ++ > > +** tags=<bool-value>: Instead of only considering annotated tags, consider > > + lightweight tags as well. > > +** abbrev=<number>: Instead of using the default number of hexadecimal digits > > + (which will vary according to the number of objects in the > > + repository with a default of 7) of the abbreviated > > + object name, use <number> digits, or as many digits as > > + needed to form a unique object name. > > +** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, > > + excluding the "refs/tags/" prefix. > > +** exclude=<pattern>: Do not consider tags matching the given `glob(7)` > > + pattern,excluding the "refs/tags/" prefix. > > You are missing a SP after the comma in "pattern,excluding" above. > > The above description is slightly different from what "git describe > --help" has. If they are described differently on purpose (e.g. you > may have made "%(describe:abbrev=0)" not to show only the closest > tag, unlike "git describe --abbrev=0"), the differences should be > spelled out more explicitly. If the behaviours of the option here > and the corresponding one there are meant to be the same, then > either using exactly the same text, or a much abbreviated > description with a note referring to the option description of "git > describe", would help the readers better. E.g. > > abbrev=<number>;; use at least <number> hexadecimal digits; see > the corresponding option in linkgit:git-describe[1] for details. > > which would make it clear that no behavioral differences are meant. > > This new section becomes a part of an existing "labeled list" > (asciidoctor calls the construct "description list"). Starting the > new heading this patch adds with 'describe[:options]::' makes sense. > It is in line with the existing text. > > I however think that the list of options is better done as a nested > description list. Documentation/config/color.txt has an example you > can imitate. See how slots of color.grep.<slot> are described > there. > > https://docs.asciidoctor.org/asciidoc/latest/lists/description/ > https://asciidoc-py.github.io/userguide.html#_labeled_lists I read through these and looked at the examples (on the git-scm website where this is used as well). I'll make the changes. Thanks for this. > > diff --git a/ref-filter.c b/ref-filter.c > > index e0d03a9f8e..6ec647c81f 100644 > > --- a/ref-filter.c > > +++ b/ref-filter.c > > @@ -5,6 +5,7 @@ > > #include "gpg-interface.h" > > #include "hex.h" > > #include "parse-options.h" > > +#include "run-command.h" > > #include "refs.h" > > #include "wildmatch.h" > > #include "object-name.h" > > @@ -146,6 +147,7 @@ enum atom_type { > > ATOM_TAGGERDATE, > > ATOM_CREATOR, > > ATOM_CREATORDATE, > > + ATOM_DESCRIBE, > > ATOM_SUBJECT, > > ATOM_BODY, > > ATOM_TRAILERS, > > @@ -215,6 +217,13 @@ static struct used_atom { > > struct email_option { > > enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; > > } email_option; > > + struct { > > + enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE, > > + D_MATCH } option; > > + unsigned int tagbool; > > + unsigned int length; > > The name "tagbool" sounds strange, as we are not saying > "lengthint". Yeah, it does sound strange now that you take the example of "lenghtint". I'm thinking "use_tags" might be good, but I don't know. > > + char *pattern; > > + } describe; > > I am a bit confused by this structure, actually, as I cannot quite > guess from the data structure alone how you intend to use it. Does > this give a good representation for the piece of data you are trying > to capture? > > For example, %(describe:tags=no,abbrev=4) would become a single atom > with 0 in .tagbool and 4 in .length, but what does the .option > member get? That is true. After I read your email, I started thinking of redesigning it. My initial thought on doing this was to mimic the other atoms' design in "struct used_atom" but now that I have read your email, it seems that such a design doesn't work. Maybe I could do something like how it is handled in pretty while also not losing the design of other atoms in "struct used_atom". > > @@ -462,6 +471,66 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato > > return 0; > > } > > > > +static int parse_describe_option(const char *arg) > > +{ > > + if (!arg) > > + return D_BARE; > > + else if (starts_with(arg, "tags")) > > + return D_TAGS; > > + else if (starts_with(arg, "abbrev")) > > + return D_ABBREV; > > + else if(starts_with(arg, "exclude")) > > + return D_EXCLUDE; > > + else if (starts_with(arg, "match")) > > + return D_MATCH; > > + return -1; > > +} > > + > > +static int describe_atom_parser(struct ref_format *format UNUSED, > > + struct used_atom *atom, > > + const char *arg, struct strbuf *err) > > +{ > > + int opt = parse_describe_option(arg); > > + > > + switch (opt) { > > + case D_BARE: > > + break; > > + case D_TAGS: > > + /* > > + * It is also possible to just use describe:tags, which > > + * is just treated as describe:tags=1 > > + */ > > + if (skip_prefix(arg, "tags=", &arg)) { > > + if (strtoul_ui(arg, 10, &atom->u.describe.tagbool)) > > This is not how you accept a Boolean. > > "1", "0", "yes", "no", "true", "false", "on", "off" are all valid > values and you use git_parse_maybe_bool() to parse them. Will make this change. > > + return strbuf_addf_ret(err, -1, _("boolean value " > > + "expected describe:tags=%s"), arg); > > + > > + } else { > > + atom->u.describe.tagbool = 1; > > + } > > + break; > > + case D_ABBREV: > > + skip_prefix(arg, "abbrev=", &arg); > > + if (strtoul_ui(arg, 10, &atom->u.describe.length)) > > + return strbuf_addf_ret(err, -1, _("positive value " > > + "expected describe:abbrev=%s"), arg); > > + break; > > + case D_EXCLUDE: > > + skip_prefix(arg, "exclude=", &arg); > > + atom->u.describe.pattern = xstrdup(arg); > > + break; > > + case D_MATCH: > > + skip_prefix(arg, "match=", &arg); > > + atom->u.describe.pattern = xstrdup(arg); > > + break; > > + default: > > + return err_bad_arg(err, "describe", arg); > > + break; > > + } > > + atom->u.describe.option = opt; > > + return 0; > > +} > > Even though the documentation patch we saw earlier said "may be > followed by a colon and zero or more comma-separated options", this > seems to expect only and exactly one option. Indeed, if we run the > resulting git like "git for-each-ref --format='%(describe:tags=0,abbrev=4)'" > you will get complaints from this parser. > > The implementation needs redesigning as the data structure is not > equipped to handle more than one options given at the same time, as > we saw earlier. > > > @@ -664,6 +733,7 @@ static struct { > > [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, > > [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, > > [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, > > + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, > > [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, > > [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, > > [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, > > @@ -1483,6 +1553,78 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size > > } > > } > > > > +static void grab_describe_values(struct atom_value *val, int deref, > > + struct object *obj) > > +{ > > + struct commit *commit = (struct commit *)obj; > > + int i; > > + > > + for (i = 0; i < used_atom_cnt; i++) { > > + struct used_atom *atom = &used_atom[i]; > > + const char *name = atom->name; > > + struct atom_value *v = &val[i]; > > + int opt; > > + > > + struct child_process cmd = CHILD_PROCESS_INIT; > > + struct strbuf out = STRBUF_INIT; > > + struct strbuf err = STRBUF_INIT; > > + > > + if (!!deref != (*name == '*')) > > + continue; > > + if (deref) > > + name++; > > + > > + if (!skip_prefix(name, "describe", &name) || > > + (*name && *name != ':')) > > + continue; > > This looks overly expensive. Why aren't we looking at the atom_type > and see if it is ATOM_DESCRIBE here? > > > + switch(opt) { > > + case D_BARE: > > + break; > > + case D_TAGS: > > + if (atom->u.describe.tagbool) > > + strvec_push(&cmd.args, "--tags"); > > + else > > + strvec_push(&cmd.args, "--no-tags"); > > + break; > > + case D_ABBREV: > > + strvec_pushf(&cmd.args, "--abbrev=%d", > > + atom->u.describe.length); > > + break; > > + case D_EXCLUDE: > > + strvec_pushf(&cmd.args, "--exclude=%s", > > + atom->u.describe.pattern); > > + break; > > + case D_MATCH: > > + strvec_pushf(&cmd.args, "--match=%s", > > + atom->u.describe.pattern); > > + break; > > + } > > Again, it is apparent here that the atom takes only one option at most. Yeah. I'll reroll with the necessary changes so that we handle many options like the Documentation patch says and also the other things you pointed out. Thanks ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/2] t6300: run describe atom tests on a different repo 2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu @ 2023-07-05 17:57 ` Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu 2 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-05 17:57 UTC (permalink / raw) To: git; +Cc: Rene Scharfe, Kousik Sanagavarapu, Christian Couder, Hariom Verma The tests for "describe" atom and its friends currently run on the main repo of t6300, expect for the test for "bare describe", which is run on "describe-repo". Things can get messy with the other tests when such changes to a repo are done (for example, a new commit or a tag is introduced), especially in t6300 where the tests depend on commits and tags. An example for this can be seen in [1], where writing the tests the current way fails the test "Verify sorts with raw:size" on linux-sha256. This, at first glance, seems totally unrelated. Digging in a bit deeper, it is apparent that this behavior is because of the changes in the repo introduced when writing the "describe" tests, which changes the raw:size of an object. Such a change in raw-size would have been, however, small if we were dealing with SHA1, but since we are dealing with SHA256, the change in raw:size is so significant that it fails the above mentioned test. So, run all the "describe" atom tests on "describe-repo", which doesn't interfere with the main repo on which the tests in t6300 are run. [1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427 Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 44 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 98ea37d336..181b04e699 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' -test_expect_success 'describe atom vs git describe' ' - test_when_finished "rm -rf describe-repo" && - +test_expect_success 'setup for describe atom tests' ' git init describe-repo && ( cd describe-repo && @@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' ' git tag tagone && test_commit --no-tag two && - git tag -a -m "tag two" tagtwo && + git tag -a -m "tag two" tagtwo + ) +' + +test_expect_success 'describe atom vs git describe' ' + ( + cd describe-repo && - git for-each-ref refs/tags/ --format="%(objectname)" >obj && + git for-each-ref --format="%(objectname)" \ + refs/tags/ >obj && while read hash do if desc=$(git describe $hash) @@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' ' ' test_expect_success 'describe:tags vs describe --tags' ' - test_when_finished "git tag -d tagname" && - git tag tagname && - git describe --tags >expect && - git for-each-ref --format="%(describe:tags)" refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' - test_when_finished "git tag -d tagname" && - - # Case 1: We have commits between HEAD and the most - # recent tag reachable from it - test_commit --no-tag file && - git describe --abbrev=14 >expect && - git for-each-ref --format="%(describe:abbrev=14)" \ - refs/heads/ >actual && - test_cmp expect actual && - - # Make sure the hash used is atleast 14 digits long - sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && - test 15 -le $(wc -c <hexpart) && - - # Case 2: We have a tag at HEAD, describe directly gives - # the name of the tag - git tag -a -m tagged tagname && - git describe --abbrev=14 >expect && - git for-each-ref --format="%(describe:abbrev=14)" \ - refs/heads/ >actual && - test_cmp expect actual && - test tagname = $(cat actual) + ( + cd describe-repo && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + test tagname = $(cat actual) + ) ' test_expect_success 'describe:match=... vs describe --match ...' ' - test_when_finished "git tag -d tag-match" && - git tag -a -m "tag match" tag-match && - git describe --match "*-match" >expect && - git for-each-ref --format="%(describe:match="*-match")" \ - refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git tag -a -m "tag match" tag-match && + git describe --match "*-match" >expect && + git for-each-ref --format="%(describe:match="*-match")" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' test_expect_success 'describe:exclude:... vs describe --exclude ...' ' - test_when_finished "git tag -d tag-exclude" && - git tag -a -m "tag exclude" tag-exclude && - git describe --exclude "*-exclude" >expect && - git for-each-ref --format="%(describe:exclude="*-exclude")" \ - refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git tag -a -m "tag exclude" tag-exclude && + git describe --exclude "*-exclude" >expect && + git for-each-ref --format="%(describe:exclude="*-exclude")" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' cat >expected <<\EOF -- 2.41.0.237.g2d10a112d6.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 0/3] Add new "describe" atom 2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu 2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu @ 2023-07-14 19:20 ` Kousik Sanagavarapu 2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu ` (3 more replies) 2 siblings, 4 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu Hi, This patch series addresses the previous comments and so now we can do for example git for-each-ref --format="%(describe:tags=yes,abbrev=14)" PATCH 1/3 - This is a new commit which introduces two new functions for handling multiple options in ref-filter. There are two ways to do this - We change the functions in pretty so that they can be used generally and not only for placeholders. - We introduce corresponding functions in ref-filter for handling atoms. This patch follows the second approach but the first approach is also good because we don't duplicate the code. Or maybe there is a much better approach that I don't see. PATCH 2/3 - Changes are made so that we can handle multiple options and also the related docs are a nested description list. PATCH 3/3 - This commit is left unchanged. Kousik Sanagavarapu (3): ref-filter: add multiple-option parsing functions ref-filter: add new "describe" atom t6300: run describe atom tests on a different repo Documentation/git-for-each-ref.txt | 23 ++++ ref-filter.c | 206 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 98 ++++++++++++++ 3 files changed, 327 insertions(+) Range-diff against v1: -: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing functions 1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. -+describe[:options]:: human-readable name, like ++describe[:options]:: Human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ -+** tags=<bool-value>: Instead of only considering annotated tags, consider -+ lightweight tags as well. -+** abbrev=<number>: Instead of using the default number of hexadecimal digits -+ (which will vary according to the number of objects in the -+ repository with a default of 7) of the abbreviated -+ object name, use <number> digits, or as many digits as -+ needed to form a unique object name. -+** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, -+ excluding the "refs/tags/" prefix. -+** exclude=<pattern>: Do not consider tags matching the given `glob(7)` -+ pattern,excluding the "refs/tags/" prefix. ++-- ++tags=<bool-value>;; Instead of only considering annotated tags, consider ++ lightweight tags as well; see the corresponding option ++ in linkgit:git-describe[1] for details. ++abbrev=<number>;; Use at least <number> hexadecimal digits; see ++ the corresponding option in linkgit:git-describe[1] ++ for details. ++match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, ++ excluding the "refs/tags/" prefix; see the corresponding ++ option in linkgit:git-describe[1] for details. ++exclude=<pattern>;; Do not consider tags matching the given `glob(7)` ++ pattern, excluding the "refs/tags/" prefix; see the ++ corresponding option in linkgit:git-describe[1] for ++ details. ++-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: ## ref-filter.c ## @@ + #include "alloc.h" + #include "environment.h" + #include "gettext.h" ++#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" @@ ref-filter.c: static struct used_atom { enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; } email_option; + struct { -+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE, -+ D_MATCH } option; -+ unsigned int tagbool; -+ unsigned int length; -+ char *pattern; ++ enum { D_BARE, D_TAGS, D_ABBREV, ++ D_EXCLUDE, D_MATCH } option; ++ const char **args; + } describe; struct refname_atom refname; char *head; @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct return 0; } -+static int parse_describe_option(const char *arg) -+{ -+ if (!arg) -+ return D_BARE; -+ else if (starts_with(arg, "tags")) -+ return D_TAGS; -+ else if (starts_with(arg, "abbrev")) -+ return D_ABBREV; -+ else if(starts_with(arg, "exclude")) -+ return D_EXCLUDE; -+ else if (starts_with(arg, "match")) -+ return D_MATCH; -+ return -1; -+} -+ +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ -+ int opt = parse_describe_option(arg); ++ const char *describe_opts[] = { ++ "", ++ "tags", ++ "abbrev", ++ "match", ++ "exclude", ++ NULL ++ }; ++ ++ struct strvec args = STRVEC_INIT; ++ for (;;) { ++ int found = 0; ++ const char *argval; ++ size_t arglen = 0; ++ int optval = 0; ++ int opt; ++ ++ if (!arg) ++ break; ++ ++ for (opt = D_BARE; !found && describe_opts[opt]; opt++) { ++ switch(opt) { ++ case D_BARE: ++ /* ++ * Do nothing. This is the bare describe ++ * atom and we already handle this above. ++ */ ++ break; ++ case D_TAGS: ++ if (match_atom_bool_arg(arg, describe_opts[opt], ++ &arg, &optval)) { ++ if (!optval) ++ strvec_pushf(&args, "--no-%s", ++ describe_opts[opt]); ++ else ++ strvec_pushf(&args, "--%s", ++ describe_opts[opt]); ++ found = 1; ++ } ++ break; ++ case D_ABBREV: ++ if (match_atom_arg_value(arg, describe_opts[opt], ++ &arg, &argval, &arglen)) { ++ char *endptr; ++ int ret = 0; + -+ switch (opt) { -+ case D_BARE: -+ break; -+ case D_TAGS: -+ /* -+ * It is also possible to just use describe:tags, which -+ * is just treated as describe:tags=1 -+ */ -+ if (skip_prefix(arg, "tags=", &arg)) { -+ if (strtoul_ui(arg, 10, &atom->u.describe.tagbool)) -+ return strbuf_addf_ret(err, -1, _("boolean value " -+ "expected describe:tags=%s"), arg); ++ if (!arglen) ++ ret = -1; ++ if (strtol(argval, &endptr, 10) < 0) ++ ret = -1; ++ if (endptr - argval != arglen) ++ ret = -1; + -+ } else { -+ atom->u.describe.tagbool = 1; ++ if (ret) ++ return strbuf_addf_ret(err, ret, ++ _("positive value expected describe:abbrev=%s"), argval); ++ strvec_pushf(&args, "--%s=%.*s", ++ describe_opts[opt], ++ (int)arglen, argval); ++ found = 1; ++ } ++ break; ++ case D_MATCH: ++ case D_EXCLUDE: ++ if (match_atom_arg_value(arg, describe_opts[opt], ++ &arg, &argval, &arglen)) { ++ if (!arglen) ++ return strbuf_addf_ret(err, -1, ++ _("value expected describe:%s="), describe_opts[opt]); ++ strvec_pushf(&args, "--%s=%.*s", ++ describe_opts[opt], ++ (int)arglen, argval); ++ found = 1; ++ } ++ break; ++ } + } -+ break; -+ case D_ABBREV: -+ skip_prefix(arg, "abbrev=", &arg); -+ if (strtoul_ui(arg, 10, &atom->u.describe.length)) -+ return strbuf_addf_ret(err, -1, _("positive value " -+ "expected describe:abbrev=%s"), arg); -+ break; -+ case D_EXCLUDE: -+ skip_prefix(arg, "exclude=", &arg); -+ atom->u.describe.pattern = xstrdup(arg); -+ break; -+ case D_MATCH: -+ skip_prefix(arg, "match=", &arg); -+ atom->u.describe.pattern = xstrdup(arg); -+ break; -+ default: -+ return err_bad_arg(err, "describe", arg); -+ break; ++ if (!found) ++ break; + } -+ atom->u.describe.option = opt; ++ atom->u.describe.args = strvec_detach(&args); + return 0; +} + @@ ref-filter.c: static void append_lines(struct strbuf *out, const char *buf, unsi + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; ++ enum atom_type type = atom->atom_type; + const char *name = atom->name; + struct atom_value *v = &val[i]; -+ int opt; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + ++ if (type != ATOM_DESCRIBE) ++ continue; ++ + if (!!deref != (*name == '*')) + continue; + if (deref) @@ ref-filter.c: static void append_lines(struct strbuf *out, const char *buf, unsi + else + name++; + -+ opt = parse_describe_option(name); -+ if (opt < 0) -+ continue; -+ + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); -+ -+ switch(opt) { -: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing functions 1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. -+describe[:options]:: human-readable name, like ++describe[:options]:: Human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ -+** tags=<bool-value>: Instead of only considering annotated tags, consider -+ lightweight tags as well. -+** abbrev=<number>: Instead of using the default number of hexadecimal digits -+ (which will vary according to the number of objects in the -+ repository with a default of 7) of the abbreviated -+ object name, use <number> digits, or as many digits as -+ needed to form a unique object name. -+** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, -+ excluding the "refs/tags/" prefix. -+** exclude=<pattern>: Do not consider tags matching the given `glob(7)` -+ pattern,excluding the "refs/tags/" prefix. ++-- ++tags=<bool-value>;; Instead of only considering annotated tags, consider ++ lightweight tags as well; see the corresponding option ++ in linkgit:git-describe[1] for details. ++abbrev=<number>;; Use at least <number> hexadecimal digits; see ++ the corresponding option in linkgit:git-describe[1] ++ for details. ++match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, ++ excluding the "refs/tags/" prefix; see the corresponding ++ option in linkgit:git-describe[1] for details. ++exclude=<pattern>;; Do not consider tags matching the given `glob(7)` ++ pattern, excluding the "refs/tags/" prefix; see the ++ corresponding option in linkgit:git-describe[1] for ++ details. ++-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: ## ref-filter.c ## @@ + #include "alloc.h" + #include "environment.h" + #include "gettext.h" ++#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" @@ ref-filter.c: static struct used_atom { enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; } email_option; + struct { -+ enum { D_BARE, D_TAGS, D_ABBREV, D_EXCLUDE, -+ D_MATCH } option; -+ unsigned int tagbool; -+ unsigned int length; -+ char *pattern; ++ enum { D_BARE, D_TAGS, D_ABBREV, ++ D_EXCLUDE, D_MATCH } option; ++ const char **args; + } describe; struct refname_atom refname; char *head; @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct return 0; } -+static int parse_describe_option(const char *arg) -+{ -+ if (!arg) -+ return D_BARE; -+ else if (starts_with(arg, "tags")) -+ return D_TAGS; -+ else if (starts_with(arg, "abbrev")) -+ return D_ABBREV; -+ else if(starts_with(arg, "exclude")) -+ return D_EXCLUDE; -+ else if (starts_with(arg, "match")) -+ return D_MATCH; -+ return -1; -+} -+ +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ -+ int opt = parse_describe_option(arg); ++ const char *describe_opts[] = { ++ "", ++ "tags", ++ "abbrev", ++ "match", ++ "exclude", ++ NULL ++ }; ++ ++ struct strvec args = STRVEC_INIT; ++ for (;;) { ++ int found = 0; ++ const char *argval; ++ size_t arglen = 0; ++ int optval = 0; ++ int opt; ++ ++ if (!arg) ++ break; ++ ++ for (opt = D_BARE; !found && describe_opts[opt]; opt++) { ++ switch(opt) { ++ case D_BARE: ++ /* ++ * Do nothing. This is the bare describe ++ * atom and we already handle this above. ++ */ ++ break; ++ case D_TAGS: ++ if (match_atom_bool_arg(arg, describe_opts[opt], ++ &arg, &optval)) { ++ if (!optval) ++ strvec_pushf(&args, "--no-%s", ++ describe_opts[opt]); ++ else ++ strvec_pushf(&args, "--%s", ++ describe_opts[opt]); ++ found = 1; ++ } ++ break; ++ case D_ABBREV: ++ if (match_atom_arg_value(arg, describe_opts[opt], ++ &arg, &argval, &arglen)) { ++ char *endptr; ++ int ret = 0; + -+ switch (opt) { -+ case D_BARE: -+ break; -+ case D_TAGS: -+ /* -+ * It is also possible to just use describe:tags, which -+ * is just treated as describe:tags=1 -+ */ -+ if (skip_prefix(arg, "tags=", &arg)) { -+ if (strtoul_ui(arg, 10, &atom->u.describe.tagbool)) -+ return strbuf_addf_ret(err, -1, _("boolean value " -+ "expected describe:tags=%s"), arg); ++ if (!arglen) ++ ret = -1; ++ if (strtol(argval, &endptr, 10) < 0) ++ ret = -1; ++ if (endptr - argval != arglen) ++ ret = -1; + -+ } else { -+ atom->u.describe.tagbool = 1; ++ if (ret) ++ return strbuf_addf_ret(err, ret, ++ _("positive value expected describe:abbrev=%s"), argval); ++ strvec_pushf(&args, "--%s=%.*s", ++ describe_opts[opt], ++ (int)arglen, argval); ++ found = 1; ++ } ++ break; ++ case D_MATCH: ++ case D_EXCLUDE: ++ if (match_atom_arg_value(arg, describe_opts[opt], ++ &arg, &argval, &arglen)) { ++ if (!arglen) ++ return strbuf_addf_ret(err, -1, ...skipping... -+ case D_BARE: -+ break; -+ case D_TAGS: -+ if (atom->u.describe.tagbool) -+ strvec_push(&cmd.args, "--tags"); -+ else -+ strvec_push(&cmd.args, "--no-tags"); -+ break; -+ case D_ABBREV: -+ strvec_pushf(&cmd.args, "--abbrev=%d", -+ atom->u.describe.length); -+ break; -+ case D_EXCLUDE: -+ strvec_pushf(&cmd.args, "--exclude=%s", -+ atom->u.describe.pattern); -+ break; -+ case D_MATCH: -+ strvec_pushf(&cmd.args, "--match=%s", -+ atom->u.describe.pattern); -+ break; -+ } -+ ++ strvec_pushv(&cmd.args, atom->u.describe.args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); 2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a different repo fivlite BR describe4 ~ | Documents | git git checkout master Switched to branch 'master' Your branch is up to date with 'upstream/master'. fivlite BR master ~ | Documents | git git range-diff 9748a6820043..describe4 master..describe5 -: ---------- > 1: 50497067a3 ref-filter: add multiple-option parsing functions 1: 9e3e652659 ! 2: f6f882884c ref-filter: add new "describe" atom @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. -+describe[:options]:: human-readable name, like ++describe[:options]:: Human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ -+** tags=<bool-value>: Instead of only considering annotated tags, consider -+ lightweight tags as well. -+** abbrev=<number>: Instead of using the default number of hexadecimal digits -+ (which will vary according to the number of objects in the -+ repository with a default of 7) of the abbreviated -+ object name, use <number> digits, or as many digits as -+ needed to form a unique object name. -+** match=<pattern>: Only consider tags matching the given `glob(7)` pattern, -+ excluding the "refs/tags/" prefix. -+** exclude=<pattern>: Do not consider tags matching the given `glob(7)` -+ pattern,excluding the "refs/tags/" prefix. ++-- ++tags=<bool-value>;; Instead of only considering annotated tags, consider ++ lightweight tags as well; see the corresponding option ++ in linkgit:git-describe[1] for details. ++abbrev=<number>;; Use at least <number> hexadecimal digits; see ++ the corresponding option in linkgit:git-describe[1] ++ for details. ++match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, ++ excluding the "refs/tags/" prefix; see the corresponding ...skipping... -+ case D_BARE: -+ break; -+ case D_TAGS: -+ if (atom->u.describe.tagbool) -+ strvec_push(&cmd.args, "--tags"); -+ else -+ strvec_push(&cmd.args, "--no-tags"); -+ break; -+ case D_ABBREV: -+ strvec_pushf(&cmd.args, "--abbrev=%d", -+ atom->u.describe.length); -+ break; -+ case D_EXCLUDE: -+ strvec_pushf(&cmd.args, "--exclude=%s", -+ atom->u.describe.pattern); -+ break; -+ case D_MATCH: -+ strvec_pushf(&cmd.args, "--match=%s", -+ atom->u.describe.pattern); -+ break; -+ } -+ ++ strvec_pushv(&cmd.args, atom->u.describe.args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); 2: 43cd3eef3c = 3: a5122bf5e2 t6300: run describe atom tests on a different repo -- 2.41.0.321.g26b82700c0.dirty ^ permalink raw reply [flat|nested] 38+ messages in thread
* [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu @ 2023-07-14 19:20 ` Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu ` (2 subsequent siblings) 3 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma The functions match_placeholder_arg_value() match_placeholder_bool_arg() were added in pretty (4f732e0fd7 (pretty: allow %(trailers) options with explicit value, 2019-01-29)) to parse multiple options in an argument to --pretty. For example, git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" will output all the trailers matching the key and seperates them by commas per commit. Add similar functions, match_atom_arg_value() match_atom_bool_arg() in ref-filter. A particular use of this can be seen in the subsequent commit where we parse the options given to a new atom "describe". Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index e0d03a9f8e..b170994d9d 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -251,6 +251,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg) return -1; } +static int match_atom_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) +{ + const char *atom; + + if (!(skip_prefix(to_parse, candidate, &atom))) + return 0; + if (valuestart) { + if (*atom == '=') { + *valuestart = atom + 1; + *valuelen = strcspn(*valuestart, ",\0"); + atom = *valuestart + *valuelen; + } else { + if (*atom != ',' && *atom != '\0') + return 0; + *valuestart = NULL; + *valuelen = 0; + } + } + if (*atom == ',') { + *end = atom + 1; + return 1; + } + if (*atom == '\0') { + *end = atom; + return 1; + } + return 0; +} + +static int match_atom_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *argval; + char *strval; + size_t arglen; + int v; + + if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen)) + return 0; + + if (!argval) { + *val = 1; + return 1; + } + + strval = xstrndup(argval, arglen); + v = git_parse_maybe_bool(strval); + free(strval); + + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static int color_atom_parser(struct ref_format *format, struct used_atom *atom, const char *color_value, struct strbuf *err) { -- 2.41.0.321.g26b82700c0.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 2/3] ref-filter: add new "describe" atom 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu 2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-14 19:20 ` Kousik Sanagavarapu 2023-07-14 20:57 ` Junio C Hamano 2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu 3 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma Duplicate the logic of %(describe) and friends from pretty to ref-filter. In the future, this change helps in unifying both the formats as ref-filter will be able to do everything that pretty is doing and we can have a single interface. The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/git-for-each-ref.txt | 23 +++++ ref-filter.c | 147 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 85 +++++++++++++++++ 3 files changed, 255 insertions(+) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1e215d4e73..2a44119f38 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -231,6 +231,29 @@ ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. +describe[:options]:: Human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ +-- +tags=<bool-value>;; Instead of only considering annotated tags, consider + lightweight tags as well; see the corresponding option + in linkgit:git-describe[1] for details. +abbrev=<number>;; Use at least <number> hexadecimal digits; see + the corresponding option in linkgit:git-describe[1] + for details. +match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding + option in linkgit:git-describe[1] for details. +exclude=<pattern>;; Do not consider tags matching the given `glob(7)` + pattern, excluding the "refs/tags/" prefix; see the + corresponding option in linkgit:git-describe[1] for + details. +-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/ref-filter.c b/ref-filter.c index b170994d9d..fe4830dbea 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2,9 +2,11 @@ #include "alloc.h" #include "environment.h" #include "gettext.h" +#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" +#include "run-command.h" #include "refs.h" #include "wildmatch.h" #include "object-name.h" @@ -146,6 +148,7 @@ enum atom_type { ATOM_TAGGERDATE, ATOM_CREATOR, ATOM_CREATORDATE, + ATOM_DESCRIBE, ATOM_SUBJECT, ATOM_BODY, ATOM_TRAILERS, @@ -215,6 +218,11 @@ static struct used_atom { struct email_option { enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; } email_option; + struct { + enum { D_BARE, D_TAGS, D_ABBREV, + D_EXCLUDE, D_MATCH } option; + const char **args; + } describe; struct refname_atom refname; char *head; } u; @@ -521,6 +529,94 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato return 0; } +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + const char *describe_opts[] = { + "", + "tags", + "abbrev", + "match", + "exclude", + NULL + }; + + struct strvec args = STRVEC_INIT; + for (;;) { + int found = 0; + const char *argval; + size_t arglen = 0; + int optval = 0; + int opt; + + if (!arg) + break; + + for (opt = D_BARE; !found && describe_opts[opt]; opt++) { + switch(opt) { + case D_BARE: + /* + * Do nothing. This is the bare describe + * atom and we already handle this above. + */ + break; + case D_TAGS: + if (match_atom_bool_arg(arg, describe_opts[opt], + &arg, &optval)) { + if (!optval) + strvec_pushf(&args, "--no-%s", + describe_opts[opt]); + else + strvec_pushf(&args, "--%s", + describe_opts[opt]); + found = 1; + } + break; + case D_ABBREV: + if (match_atom_arg_value(arg, describe_opts[opt], + &arg, &argval, &arglen)) { + char *endptr; + int ret = 0; + + if (!arglen) + ret = -1; + if (strtol(argval, &endptr, 10) < 0) + ret = -1; + if (endptr - argval != arglen) + ret = -1; + + if (ret) + return strbuf_addf_ret(err, ret, + _("positive value expected describe:abbrev=%s"), argval); + strvec_pushf(&args, "--%s=%.*s", + describe_opts[opt], + (int)arglen, argval); + found = 1; + } + break; + case D_MATCH: + case D_EXCLUDE: + if (match_atom_arg_value(arg, describe_opts[opt], + &arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected describe:%s="), describe_opts[opt]); + strvec_pushf(&args, "--%s=%.*s", + describe_opts[opt], + (int)arglen, argval); + found = 1; + } + break; + } + } + if (!found) + break; + } + atom->u.describe.args = strvec_detach(&args); + return 0; +} + static int raw_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) @@ -723,6 +819,7 @@ static struct { [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, @@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } } +static void grab_describe_values(struct atom_value *val, int deref, + struct object *obj) +{ + struct commit *commit = (struct commit *)obj; + int i; + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + enum atom_type type = atom->atom_type; + const char *name = atom->name; + struct atom_value *v = &val[i]; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + + if (type != ATOM_DESCRIBE) + continue; + + if (!!deref != (*name == '*')) + continue; + if (deref) + name++; + + if (!skip_prefix(name, "describe", &name) || + (*name && *name != ':')) + continue; + if (!*name) + name = NULL; + else + name++; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); + strvec_pushv(&cmd.args, atom->u.describe.args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); + v->s = xstrdup(""); + continue; + } + strbuf_rtrim(&out); + v->s = strbuf_detach(&out, NULL); + + strbuf_release(&err); + } +} + /* See grab_values */ static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data) { @@ -1651,12 +1796,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_tag_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("tagger", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: /* grab_tree_values(val, deref, obj, buf, sz); */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 5c00607608..98ea37d336 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -561,6 +561,91 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' +test_expect_success 'describe atom vs git describe' ' + test_when_finished "rm -rf describe-repo" && + + git init describe-repo && + ( + cd describe-repo && + + test_commit --no-tag one && + git tag tagone && + + test_commit --no-tag two && + git tag -a -m "tag two" tagtwo && + + git for-each-ref refs/tags/ --format="%(objectname)" >obj && + while read hash + do + if desc=$(git describe $hash) + then + : >expect-contains-good + else + : >expect-contains-bad + fi && + echo "$hash $desc" || return 1 + done <obj >expect && + test_path_exists expect-contains-good && + test_path_exists expect-contains-bad && + + git for-each-ref --format="%(objectname) %(describe)" \ + refs/tags/ >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + ) +' + +test_expect_success 'describe:tags vs describe --tags' ' + test_when_finished "git tag -d tagname" && + git tag tagname && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" refs/heads/ >actual && + test_cmp expect actual +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' + test_when_finished "git tag -d tagname" && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + test tagname = $(cat actual) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' + test_when_finished "git tag -d tag-match" && + git tag -a -m "tag match" tag-match && + git describe --match "*-match" >expect && + git for-each-ref --format="%(describe:match="*-match")" \ + refs/heads/ >actual && + test_cmp expect actual +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' + test_when_finished "git tag -d tag-exclude" && + git tag -a -m "tag exclude" tag-exclude && + git describe --exclude "*-exclude" >expect && + git for-each-ref --format="%(describe:exclude="*-exclude")" \ + refs/heads/ >actual && + test_cmp expect actual +' + cat >expected <<\EOF heads/main tags/main -- 2.41.0.321.g26b82700c0.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom 2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-14 20:57 ` Junio C Hamano 2023-07-15 18:24 ` Kousik Sanagavarapu 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-14 20:57 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > + struct { > + enum { D_BARE, D_TAGS, D_ABBREV, > + D_EXCLUDE, D_MATCH } option; > + const char **args; > + } describe; As you parse this into a strvec that has command line options for the "git describe" invocation, I do not see the point of having the "enum option" in this struct. The describe->option member seems to be unused throughout this patch. In fact, a single "const char **describe_args" should be able to replace the structure, no? > +static int describe_atom_parser(struct ref_format *format UNUSED, > + struct used_atom *atom, > + const char *arg, struct strbuf *err) > +{ > + const char *describe_opts[] = { > + "", > + "tags", > + "abbrev", > + "match", > + "exclude", > + NULL > + }; > + > + struct strvec args = STRVEC_INIT; > + for (;;) { > + int found = 0; > + const char *argval; > + size_t arglen = 0; > + int optval = 0; > + int opt; > + > + if (!arg) > + break; > + > + for (opt = D_BARE; !found && describe_opts[opt]; opt++) { > + switch(opt) { > + case D_BARE: > + /* > + * Do nothing. This is the bare describe > + * atom and we already handle this above. > + */ > + break; > + case D_TAGS: > + if (match_atom_bool_arg(arg, describe_opts[opt], > + &arg, &optval)) { > + if (!optval) > + strvec_pushf(&args, "--no-%s", > + describe_opts[opt]); > + else > + strvec_pushf(&args, "--%s", > + describe_opts[opt]); > + found = 1; > + } As match_atom_bool_arg() and ... > + break; > + case D_ABBREV: > + if (match_atom_arg_value(arg, describe_opts[opt], > + &arg, &argval, &arglen)) { > + char *endptr; > + int ret = 0; > + > + if (!arglen) > + ret = -1; > + if (strtol(argval, &endptr, 10) < 0) > + ret = -1; > + if (endptr - argval != arglen) > + ret = -1; > + > + if (ret) > + return strbuf_addf_ret(err, ret, > + _("positive value expected describe:abbrev=%s"), argval); > + strvec_pushf(&args, "--%s=%.*s", > + describe_opts[opt], > + (int)arglen, argval); > + found = 1; > + } ... match_atom_arg_value() are both silent when they return false, we do not see any diagnosis when these two case arms set the "found" flag. Shouldn't we have a corresponding "else" clause to these "if (match_atom_blah())" blocks to issue an error message or something? > + break; > + case D_MATCH: > + case D_EXCLUDE: > + if (match_atom_arg_value(arg, describe_opts[opt], > + &arg, &argval, &arglen)) { > + if (!arglen) > + return strbuf_addf_ret(err, -1, > + _("value expected describe:%s="), describe_opts[opt]); > + strvec_pushf(&args, "--%s=%.*s", > + describe_opts[opt], > + (int)arglen, argval); > + found = 1; > + } > + break; > + } > + } > + if (!found) > + break; > + } > + atom->u.describe.args = strvec_detach(&args); > + return 0; > +} > + > static int raw_atom_parser(struct ref_format *format UNUSED, > struct used_atom *atom, > const char *arg, struct strbuf *err) > @@ -723,6 +819,7 @@ static struct { > [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, > [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, > [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, > + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, > [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, > [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, > [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, > @@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size > } > } > > +static void grab_describe_values(struct atom_value *val, int deref, > + struct object *obj) > +{ > + struct commit *commit = (struct commit *)obj; > + int i; > + > + for (i = 0; i < used_atom_cnt; i++) { > + struct used_atom *atom = &used_atom[i]; > + enum atom_type type = atom->atom_type; > + const char *name = atom->name; > + struct atom_value *v = &val[i]; > + > + struct child_process cmd = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct strbuf err = STRBUF_INIT; > + > + if (type != ATOM_DESCRIBE) > + continue; We already have parsed the %(describe:...) and the result is stored in the used_atom[] array. We iterate over the array, and we just found that its atom_type member is ATOM_DESCRIBE here (otherwise we would have moved on to the next array element). > + if (!!deref != (*name == '*')) > + continue; This is trying to avoid %(*describe) answering when the given object is the tag itself, or %(describe) answering when the given object is what the tag dereferences to, so having it here makes sense (by the way, do you add any test for "%(*describe)?"). Now, is the code from here ... > + if (deref) > + name++; > + > + if (!skip_prefix(name, "describe", &name) || > + (*name && *name != ':')) > + continue; > + if (!*name) > + name = NULL; > + else > + name++; ... down to here doing anything useful? After all, you already have all you need to describe the commit in atom->u.describe_args to run "git describe" with, no? In fact, after computing "name" with the above code with some complexity, nobody even looks at it. Perhaps the above was copied from some other grab_* functions; the reason why they were relevant there needs to be understood, and it also has to be considered if the same reason to have the code here applies to this codepath. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom 2023-07-14 20:57 ` Junio C Hamano @ 2023-07-15 18:24 ` Kousik Sanagavarapu 2023-07-15 18:56 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-15 18:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma On Fri, Jul 14, 2023 at 01:57:40PM -0700, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > + struct { > > + enum { D_BARE, D_TAGS, D_ABBREV, > > + D_EXCLUDE, D_MATCH } option; > > + const char **args; > > + } describe; > > As you parse this into a strvec that has command line options for > the "git describe" invocation, I do not see the point of having the > "enum option" in this struct. The describe->option member seems to > be unused throughout this patch. > > In fact, a single "const char **describe_args" should be able to > replace the structure, no? I kept the enum because I thought it could act as an index for the describe_opts array. Now that I think about it, diff --git a/ref-filter.c b/ref-filter.c index fe4830dbea..df7cb39be2 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -219,9 +219,7 @@ static struct used_atom { enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; } email_option; struct { - enum { D_BARE, D_TAGS, D_ABBREV, - D_EXCLUDE, D_MATCH } option; - const char **args; + const char **decsribe_args; } describe; struct refname_atom refname; char *head; @@ -533,13 +531,16 @@ static int describe_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) { - const char *describe_opts[] = { - "", - "tags", - "abbrev", - "match", - "exclude", - NULL + struct { + char *optname; + enum { D_BARE, D_TAGS, D_ABBREV, + D_MATCH, D_EXCLUDE } option; + } describe_opts[] = { + { "", D_BARE }, + { "tags", D_TAGS }, + { "abbrev", D_ABBREV }, + { "match", D_MATCH }, + { "exclude", D_EXCLUDE } }; struct strvec args = STRVEC_INIT; conveys it better or is it too much unnecessary stuff to and should we just do struct { const char **describe_args; } describe; leaving the describe_opts array as is and changing the how the switch is written. > > +static int describe_atom_parser(struct ref_format *format UNUSED, > > + struct used_atom *atom, > > + const char *arg, struct strbuf *err) > > +{ > > + const char *describe_opts[] = { > > + "", > > + "tags", > > + "abbrev", > > + "match", > > + "exclude", > > + NULL > > + }; > > + > > + struct strvec args = STRVEC_INIT; > > + for (;;) { > > + int found = 0; > > + const char *argval; > > + size_t arglen = 0; > > + int optval = 0; > > + int opt; > > + > > + if (!arg) > > + break; > > + > > + for (opt = D_BARE; !found && describe_opts[opt]; opt++) { > > + switch(opt) { > > + case D_BARE: > > + /* > > + * Do nothing. This is the bare describe > > + * atom and we already handle this above. > > + */ > > + break; > > + case D_TAGS: > > + if (match_atom_bool_arg(arg, describe_opts[opt], > > + &arg, &optval)) { > > + if (!optval) > > + strvec_pushf(&args, "--no-%s", > > + describe_opts[opt]); > > + else > > + strvec_pushf(&args, "--%s", > > + describe_opts[opt]); > > + found = 1; > > + } > > As match_atom_bool_arg() and ... > > > + break; > > + case D_ABBREV: > > + if (match_atom_arg_value(arg, describe_opts[opt], > > + &arg, &argval, &arglen)) { > > + char *endptr; > > + int ret = 0; > > + > > + if (!arglen) > > + ret = -1; > > + if (strtol(argval, &endptr, 10) < 0) > > + ret = -1; > > + if (endptr - argval != arglen) > > + ret = -1; > > + > > + if (ret) > > + return strbuf_addf_ret(err, ret, > > + _("positive value expected describe:abbrev=%s"), argval); > > + strvec_pushf(&args, "--%s=%.*s", > > + describe_opts[opt], > > + (int)arglen, argval); > > + found = 1; > > + } > > ... match_atom_arg_value() are both silent when they return false, > we do not see any diagnosis when these two case arms set the "found" > flag. Shouldn't we have a corresponding "else" clause to these "if > (match_atom_blah())" blocks to issue an error message or something? Yeah, I'll add this. > [...] > Now, is the code from here ... > > > + if (deref) > > + name++; > > + > > + if (!skip_prefix(name, "describe", &name) || > > + (*name && *name != ':')) > > + continue; > > + if (!*name) > > + name = NULL; > > + else > > + name++; > > ... down to here doing anything useful? After all, you already have > all you need to describe the commit in atom->u.describe_args to run > "git describe" with, no? In fact, after computing "name" with the > above code with some complexity, nobody even looks at it. > > Perhaps the above was copied from some other grab_* functions; the > reason why they were relevant there needs to be understood, and it > also has to be considered if the same reason to have the code here > applies to this codepath. Sorry you had to read through this. I'll remove these if constructs, because as you said, they do nothing since we already parse everything we need and also check for the type and the deref. There is not test for "%(*describe)", but I'll add one in v3 if you think it is necessary (if we are doing this, should we also do one for multiple options?). Thanks ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/3] ref-filter: add new "describe" atom 2023-07-15 18:24 ` Kousik Sanagavarapu @ 2023-07-15 18:56 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-15 18:56 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > conveys it better or is it too much unnecessary stuff to and should we > just do > > struct { > const char **describe_args; > } describe; > > leaving the describe_opts array as is and changing the how the switch is > written. I think this struct can be replaced with a single const char **describe_args; and then >> > +static int describe_atom_parser(struct ref_format *format UNUSED, >> > + struct used_atom *atom, >> > + const char *arg, struct strbuf *err) >> > +{ >> > + const char *describe_opts[] = { >> > + "", >> > + "tags", >> > + "abbrev", >> > + "match", >> > + "exclude", >> > + NULL >> > + }; this array can simply go away. Then you can >> > + struct strvec args = STRVEC_INIT; >> > + for (;;) { >> > + int found = 0; >> > + const char *argval; >> > + size_t arglen = 0; >> > + int optval = 0; >> > + int opt; >> > + >> > + if (!arg) >> > + break; >> > + >> > + for (opt = D_BARE; !found && describe_opts[opt]; opt++) { rewrite this "for" loop plus the "switch" inside to an if/else if/else cascade: if (match_atom_bool_arg(arg, "tags", &arg, &optval)) { ... do "tags" thing ... } else if (match_atom_arg_value(arg, "abbrev", ...)) { ... do "abbrev" thing ... } else if ... That way, you do not need any enum anywhere and there is no reason to have desribe_opts[] array, either. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 3/3] t6300: run describe atom tests on a different repo 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu 2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-14 19:20 ` Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu 3 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-14 19:20 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Kousik Sanagavarapu, Christian Couder, Hariom Verma The tests for "describe" atom and its friends currently run on the main repo of t6300, expect for the test for "bare describe", which is run on "describe-repo". Things can get messy with the other tests when such changes to a repo are done (for example, a new commit or a tag is introduced), especially in t6300 where the tests depend on commits and tags. An example for this can be seen in [1], where writing the tests the current way fails the test "Verify sorts with raw:size" on linux-sha256. This, at first glance, seems totally unrelated. Digging in a bit deeper, it is apparent that this behavior is because of the changes in the repo introduced when writing the "describe" tests, which changes the raw:size of an object. Such a change in raw-size would have been, however, small if we were dealing with SHA1, but since we are dealing with SHA256, the change in raw:size is so significant that it fails the above mentioned test. So, run all the "describe" atom tests on "describe-repo", which doesn't interfere with the main repo on which the tests in t6300 are run. [1]: https://github.com/five-sh/git/actions/runs/5446892074/jobs/9908256427 Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- t/t6300-for-each-ref.sh | 101 +++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 44 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 98ea37d336..181b04e699 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -561,9 +561,7 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' -test_expect_success 'describe atom vs git describe' ' - test_when_finished "rm -rf describe-repo" && - +test_expect_success 'setup for describe atom tests' ' git init describe-repo && ( cd describe-repo && @@ -572,9 +570,16 @@ test_expect_success 'describe atom vs git describe' ' git tag tagone && test_commit --no-tag two && - git tag -a -m "tag two" tagtwo && + git tag -a -m "tag two" tagtwo + ) +' + +test_expect_success 'describe atom vs git describe' ' + ( + cd describe-repo && - git for-each-ref refs/tags/ --format="%(objectname)" >obj && + git for-each-ref --format="%(objectname)" \ + refs/tags/ >obj && while read hash do if desc=$(git describe $hash) @@ -596,54 +601,62 @@ test_expect_success 'describe atom vs git describe' ' ' test_expect_success 'describe:tags vs describe --tags' ' - test_when_finished "git tag -d tagname" && - git tag tagname && - git describe --tags >expect && - git for-each-ref --format="%(describe:tags)" refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' - test_when_finished "git tag -d tagname" && - - # Case 1: We have commits between HEAD and the most - # recent tag reachable from it - test_commit --no-tag file && - git describe --abbrev=14 >expect && - git for-each-ref --format="%(describe:abbrev=14)" \ - refs/heads/ >actual && - test_cmp expect actual && - - # Make sure the hash used is atleast 14 digits long - sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && - test 15 -le $(wc -c <hexpart) && - - # Case 2: We have a tag at HEAD, describe directly gives - # the name of the tag - git tag -a -m tagged tagname && - git describe --abbrev=14 >expect && - git for-each-ref --format="%(describe:abbrev=14)" \ - refs/heads/ >actual && - test_cmp expect actual && - test tagname = $(cat actual) + ( + cd describe-repo && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/ >actual && + test_cmp expect actual && + test tagname = $(cat actual) + ) ' test_expect_success 'describe:match=... vs describe --match ...' ' - test_when_finished "git tag -d tag-match" && - git tag -a -m "tag match" tag-match && - git describe --match "*-match" >expect && - git for-each-ref --format="%(describe:match="*-match")" \ - refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git tag -a -m "tag match" tag-match && + git describe --match "*-match" >expect && + git for-each-ref --format="%(describe:match="*-match")" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' test_expect_success 'describe:exclude:... vs describe --exclude ...' ' - test_when_finished "git tag -d tag-exclude" && - git tag -a -m "tag exclude" tag-exclude && - git describe --exclude "*-exclude" >expect && - git for-each-ref --format="%(describe:exclude="*-exclude")" \ - refs/heads/ >actual && - test_cmp expect actual + ( + cd describe-repo && + git tag -a -m "tag exclude" tag-exclude && + git describe --exclude "*-exclude" >expect && + git for-each-ref --format="%(describe:exclude="*-exclude")" \ + refs/heads/ >actual && + test_cmp expect actual + ) ' cat >expected <<\EOF -- 2.41.0.321.g26b82700c0.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 0/2] Add new "describe" atom 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu ` (2 preceding siblings ...) 2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu @ 2023-07-19 16:15 ` Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu ` (3 more replies) 3 siblings, 4 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu Hi, Thanks Junio for the review on the previous version of this series. I have made the changes according to the comments left. PATCH 1/2 - Left unchanged expect for small changes in the commit message for more clarity. PATCH 2/2 - We now parse the arguments in a seperate function `describe_atom_option_parser()` call this in `describe_atom_parser()` instead to populate `atom->u.describe_args`. This splitting of the function helps err at the right places. I've also squashed the third commit in the previous version into this commit. Kousik Sanagavarapu (2): ref-filter: add multiple-option parsing functions ref-filter: add new "describe" atom Documentation/git-for-each-ref.txt | 23 ++++ ref-filter.c | 189 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 114 +++++++++++++++++ 3 files changed, 326 insertions(+) Range-diff against v2: 1: 50497067a3 ! 1: 08f3be1631 ref-filter: add multiple-option parsing functions @@ Commit message match_placeholder_arg_value() match_placeholder_bool_arg() - were added in pretty (4f732e0fd7 (pretty: allow %(trailers) options - with explicit value, 2019-01-29)) to parse multiple options in an + were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options + with explicit value, 2019-01-29) to parse multiple options in an argument to --pretty. For example, git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" will output all the trailers matching the key and seperates them by - commas per commit. + a comma followed by a space per commit. Add similar functions, match_atom_arg_value() match_atom_bool_arg() - in ref-filter. A particular use of this can be seen in the subsequent - commit where we parse the options given to a new atom "describe". + in ref-filter. + + There is no atom yet that can use these functions in ref-filter, but we + are going to add a new %(describe) atom in a subsequent commit where we + parse options like tags=<bool-value> or match=<pattern> given to it. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> 2: f6f882884c ! 2: 742a79113c ref-filter: add new "describe" atom @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: ## ref-filter.c ## @@ - #include "alloc.h" + #include "git-compat-util.h" #include "environment.h" #include "gettext.h" +#include "config.h" @@ ref-filter.c: enum atom_type { ATOM_BODY, ATOM_TRAILERS, @@ ref-filter.c: static struct used_atom { - struct email_option { - enum { EO_RAW, EO_TRIM, EO_LOCALPART } option; - } email_option; -+ struct { -+ enum { D_BARE, D_TAGS, D_ABBREV, -+ D_EXCLUDE, D_MATCH } option; -+ const char **args; -+ } describe; + enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, + S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; + } signature; ++ const char **describe_args; struct refname_atom refname; char *head; } u; @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct return 0; } ++static int describe_atom_option_parser(struct strvec *args, const char **arg, ++ struct strbuf *err) ++{ ++ const char *argval; ++ size_t arglen = 0; ++ int optval = 0; ++ ++ if (match_atom_bool_arg(*arg, "tags", arg, &optval)) { ++ if (!optval) ++ strvec_push(args, "--no-tags"); ++ else ++ strvec_push(args, "--tags"); ++ return 1; ++ } ++ ++ if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) { ++ char *endptr; ++ ++ if (!arglen) ++ return strbuf_addf_ret(err, -1, ++ _("argument expected for %s"), ++ "describe:abbrev"); ++ if (strtol(argval, &endptr, 10) < 0) ++ return strbuf_addf_ret(err, -1, ++ _("positive value expected %s=%s"), ++ "describe:abbrev", argval); ++ if (endptr - argval != arglen) ++ return strbuf_addf_ret(err, -1, ++ _("cannot fully parse %s=%s"), ++ "describe:abbrev", argval); ++ ++ strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval); ++ return 1; ++ } ++ ++ if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) { ++ if (!arglen) ++ return strbuf_addf_ret(err, -1, ++ _("value expected %s="), ++ "describe:match"); ++ ++ strvec_pushf(args, "--match=%.*s", (int)arglen, argval); ++ return 1; ++ } ++ ++ if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) { ++ if (!arglen) ++ return strbuf_addf_ret(err, -1, ++ _("value expected %s="), ++ "describe:exclude"); ++ ++ strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval); ++ return 1; ++ } ++ ++ return 0; ++} ++ +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ -+ const char *describe_opts[] = { -+ "", -+ "tags", -+ "abbrev", -+ "match", -+ "exclude", -+ NULL -+ }; -+ + struct strvec args = STRVEC_INIT; ++ + for (;;) { + int found = 0; -+ const char *argval; -+ size_t arglen = 0; -+ int optval = 0; -+ int opt; ++ const char *bad_arg = NULL; + -+ if (!arg) ++ if (!arg || !*arg) + break; + -+ for (opt = D_BARE; !found && describe_opts[opt]; opt++) { -+ switch(opt) { -+ case D_BARE: -+ /* -+ * Do nothing. This is the bare describe -+ * atom and we already handle this above. -+ */ -+ break; -+ case D_TAGS: -+ if (match_atom_bool_arg(arg, describe_opts[opt], -+ &arg, &optval)) { -+ if (!optval) -+ strvec_pushf(&args, "--no-%s", -+ describe_opts[opt]); -+ else -+ strvec_pushf(&args, "--%s", -+ describe_opts[opt]); -+ found = 1; -+ } -+ break; -+ case D_ABBREV: -+ if (match_atom_arg_value(arg, describe_opts[opt], -+ &arg, &argval, &arglen)) { -+ char *endptr; -+ int ret = 0; -+ -+ if (!arglen) -+ ret = -1; -+ if (strtol(argval, &endptr, 10) < 0) -+ ret = -1; -+ if (endptr - argval != arglen) -+ ret = -1; -+ -+ if (ret) -+ return strbuf_addf_ret(err, ret, -+ _("positive value expected describe:abbrev=%s"), argval); -+ strvec_pushf(&args, "--%s=%.*s", -+ describe_opts[opt], -+ (int)arglen, argval); -+ found = 1; -+ } -+ break; -+ case D_MATCH: -+ case D_EXCLUDE: -+ if (match_atom_arg_value(arg, describe_opts[opt], -+ &arg, &argval, &arglen)) { -+ if (!arglen) -+ return strbuf_addf_ret(err, -1, -+ _("value expected describe:%s="), describe_opts[opt]); -+ strvec_pushf(&args, "--%s=%.*s", -+ describe_opts[opt], -+ (int)arglen, argval); -+ found = 1; -+ } -+ break; -+ } -+ } -+ if (!found) ++ bad_arg = arg; ++ found = describe_atom_option_parser(&args, &arg, err); ++ if (found < 0) ++ return found; ++ if (!found) { ++ if (bad_arg && *bad_arg) ++ return err_bad_arg(err, "describe", bad_arg); + break; ++ } + } -+ atom->u.describe.args = strvec_detach(&args); ++ atom->u.describe_args = strvec_detach(&args); + return 0; +} + @@ ref-filter.c: static void append_lines(struct strbuf *out, const char *buf, unsi + + if (!!deref != (*name == '*')) + continue; -+ if (deref) -+ name++; -+ -+ if (!skip_prefix(name, "describe", &name) || -+ (*name && *name != ':')) -+ continue; -+ if (!*name) -+ name = NULL; -+ else -+ name++; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); -+ strvec_pushv(&cmd.args, atom->u.describe.args); ++ strvec_pushv(&cmd.args, atom->u.describe_args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); @@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); - grab_sub_body_contents(val, deref, data); +@@ ref-filter.c: static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); + grab_signature(val, deref, obj); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override test_cmp expected.bare actual ' -+test_expect_success 'describe atom vs git describe' ' -+ test_when_finished "rm -rf describe-repo" && -+ ++test_expect_success 'setup for describe atom tests' ' + git init describe-repo && + ( + cd describe-repo && @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override + git tag tagone && + + test_commit --no-tag two && -+ git tag -a -m "tag two" tagtwo && ++ git tag -a -m "tag two" tagtwo ++ ) ++' + -+ git for-each-ref refs/tags/ --format="%(objectname)" >obj && ++test_expect_success 'describe atom vs git describe' ' ++ ( ++ cd describe-repo && ++ ++ git for-each-ref --format="%(objectname)" \ ++ refs/tags/ >obj && + while read hash + do + if desc=$(git describe $hash) @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override +' + +test_expect_success 'describe:tags vs describe --tags' ' -+ test_when_finished "git tag -d tagname" && -+ git tag tagname && -+ git describe --tags >expect && -+ git for-each-ref --format="%(describe:tags)" refs/heads/ >actual && -+ test_cmp expect actual ++ ( ++ cd describe-repo && ++ git describe --tags >expect && ++ git for-each-ref --format="%(describe:tags)" \ ++ refs/heads/master >actual && ++ test_cmp expect actual ++ ) +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' -+ test_when_finished "git tag -d tagname" && -+ -+ # Case 1: We have commits between HEAD and the most -+ # recent tag reachable from it -+ test_commit --no-tag file && -+ git describe --abbrev=14 >expect && -+ git for-each-ref --format="%(describe:abbrev=14)" \ -+ refs/heads/ >actual && -+ test_cmp expect actual && -+ -+ # Make sure the hash used is atleast 14 digits long -+ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && -+ test 15 -le $(wc -c <hexpart) && -+ -+ # Case 2: We have a tag at HEAD, describe directly gives -+ # the name of the tag -+ git tag -a -m tagged tagname && -+ git describe --abbrev=14 >expect && -+ git for-each-ref --format="%(describe:abbrev=14)" \ -+ refs/heads/ >actual && -+ test_cmp expect actual && -+ test tagname = $(cat actual) ++ ( ++ cd describe-repo && ++ ++ # Case 1: We have commits between HEAD and the most ++ # recent tag reachable from it ++ test_commit --no-tag file && ++ git describe --abbrev=14 >expect && ++ git for-each-ref --format="%(describe:abbrev=14)" \ ++ refs/heads/master >actual && ++ test_cmp expect actual && ++ ++ # Make sure the hash used is atleast 14 digits long ++ sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && ++ test 15 -le $(wc -c <hexpart) && ++ ++ # Case 2: We have a tag at HEAD, describe directly gives ++ # the name of the tag ++ git tag -a -m tagged tagname && ++ git describe --abbrev=14 >expect && ++ git for-each-ref --format="%(describe:abbrev=14)" \ ++ refs/heads/master >actual && ++ test_cmp expect actual && ++ test tagname = $(cat actual) ++ ) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' -+ test_when_finished "git tag -d tag-match" && -+ git tag -a -m "tag match" tag-match && -+ git describe --match "*-match" >expect && -+ git for-each-ref --format="%(describe:match="*-match")" \ -+ refs/heads/ >actual && -+ test_cmp expect actual ++ ( ++ cd describe-repo && ++ git tag -a -m "tag foo" tag-foo && ++ git describe --match "*-foo" >expect && ++ git for-each-ref --format="%(describe:match="*-foo")" \ ++ refs/heads/master >actual && ++ test_cmp expect actual ++ ) +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' -+ test_when_finished "git tag -d tag-exclude" && -+ git tag -a -m "tag exclude" tag-exclude && -+ git describe --exclude "*-exclude" >expect && -+ git for-each-ref --format="%(describe:exclude="*-exclude")" \ -+ refs/heads/ >actual && -+ test_cmp expect actual ++ ( ++ cd describe-repo && ++ git tag -a -m "tag bar" tag-bar && ++ git describe --exclude "*-bar" >expect && ++ git for-each-ref --format="%(describe:exclude="*-bar")" \ ++ refs/heads/master >actual && ++ test_cmp expect actual ++ ) ++' ++ ++test_expect_success 'deref with describe atom' ' ++ ( ++ cd describe-repo && ++ cat >expect <<-\EOF && ++ ++ tagname ++ tagname ++ tagname ++ ++ tagtwo ++ EOF ++ git for-each-ref --format="%(*describe)" >actual && ++ test_cmp expect actual ++ ) +' + cat >expected <<\EOF 3: a5122bf5e2 < -: ---------- t6300: run describe atom tests on a different repo -- 2.41.0.378.g42703afc1f.dirty ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu @ 2023-07-19 16:15 ` Kousik Sanagavarapu 2023-07-19 23:23 ` Junio C Hamano 2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu ` (2 subsequent siblings) 3 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu The functions match_placeholder_arg_value() match_placeholder_bool_arg() were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options with explicit value, 2019-01-29) to parse multiple options in an argument to --pretty. For example, git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" will output all the trailers matching the key and seperates them by a comma followed by a space per commit. Add similar functions, match_atom_arg_value() match_atom_bool_arg() in ref-filter. There is no atom yet that can use these functions in ref-filter, but we are going to add a new %(describe) atom in a subsequent commit where we parse options like tags=<bool-value> or match=<pattern> given to it. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index 60919f375f..f64437e781 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -255,6 +255,65 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg) return -1; } +static int match_atom_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) +{ + const char *atom; + + if (!(skip_prefix(to_parse, candidate, &atom))) + return 0; + if (valuestart) { + if (*atom == '=') { + *valuestart = atom + 1; + *valuelen = strcspn(*valuestart, ",\0"); + atom = *valuestart + *valuelen; + } else { + if (*atom != ',' && *atom != '\0') + return 0; + *valuestart = NULL; + *valuelen = 0; + } + } + if (*atom == ',') { + *end = atom + 1; + return 1; + } + if (*atom == '\0') { + *end = atom; + return 1; + } + return 0; +} + +static int match_atom_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *argval; + char *strval; + size_t arglen; + int v; + + if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen)) + return 0; + + if (!argval) { + *val = 1; + return 1; + } + + strval = xstrndup(argval, arglen); + v = git_parse_maybe_bool(strval); + free(strval); + + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static int color_atom_parser(struct ref_format *format, struct used_atom *atom, const char *color_value, struct strbuf *err) { -- 2.41.0.378.g42703afc1f.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-19 23:23 ` Junio C Hamano 2023-07-20 5:21 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-19 23:23 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh, Glen Choo Kousik Sanagavarapu <five231003@gmail.com> writes: > ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) New helper functions that do not have any caller and no documentation to explain how they are supposed to be called (i.e. the expectation on the callers---what values they need to feed as parameters when they call these helpers, and the expectation by the callers---what they expect to get out of the helpers once they return) makes it impossible to evaluate if they are any good [*]. Side note. Those of you who are keen to add unit tests to the system (Cc:ed) , do you think a patch line this one that adds a new helper function to the system, would benefit from being able to add a few unit tests for these otherwise unused helper functions? The calls to the new functions that the unit test framework would make should serve as a good piece of interface documentation, showing what the callers are supposed to pass and what they expect, I guess. So whatever framework we choose, it should allow adding a test or two to this patch easily, without being too intrusive. Would that be a good and concrete evaluation criterion? Anyway, because of that, I had to read [2/2] first and then come back here to review this one. The following is my attempt to write down the contract between the callers and this new helper function---please give something like that to the final version. The the example below is there just to illustrate the level of information that would be desired to help future readers and programmers. Do not take the contents as-written as truth---I may have (deliberately) mixed in incorrect descriptions ;-). /* * The string "to_parse" is expected to be a comma-separated list * of "key" or "key=val". If your atom allows "key1" and "key2" * (possibly with their values) as options, make two calls to this * funtion, passing "key1" in candiate and then passing "key2" in * candidate. * * The function Returns true ONLY when the to_parse string begins * with the candidate key, possibly followed by its value (valueless * key-only entries are allowed in the comman-separated list). * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and * the function returns false. * * *valuestart will point at the byte after '=' (i.e. the beginning * of the value), and the number of bytes in the value will be set * to *valuelen. * A key-only entry results in *valuestart set to NULL and *valuelen * set to 0. * *end will point at the next key[=val] in the comma-separated list * or NULL when the list ran out. */ > +static int match_atom_arg_value(const char *to_parse, const char *candidate, > + const char **end, const char **valuestart, > + size_t *valuelen) > +{ > + const char *atom; > + > + if (!(skip_prefix(to_parse, candidate, &atom))) > + return 0; > + if (valuestart) { As far as I saw, no callers pass NULL to valuestart. Getting rid of this if() statement and always entering its body would clarify what is going on, I think. > + if (*atom == '=') { > + *valuestart = atom + 1; > + *valuelen = strcspn(*valuestart, ",\0"); > + atom = *valuestart + *valuelen; > + } else { > + if (*atom != ',' && *atom != '\0') > + return 0; > + *valuestart = NULL; > + *valuelen = 0; > + } > + } > + if (*atom == ',') { > + *end = atom + 1; > + return 1; > + } > + if (*atom == '\0') { > + *end = atom; > + return 1; > + } > + return 0; > +} /* * Write something similar to document the contract between the caller * and this function here. */ > +static int match_atom_bool_arg(const char *to_parse, const char *candidate, > + const char **end, int *val) > +{ Thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-19 23:23 ` Junio C Hamano @ 2023-07-20 5:21 ` Junio C Hamano 2023-07-20 16:52 ` Kousik Sanagavarapu 2023-07-20 17:42 ` Glen Choo 2 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-20 5:21 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh, Glen Choo Junio C Hamano <gitster@pobox.com> writes: >> +static int match_atom_arg_value(const char *to_parse, const char *candidate, >> + const char **end, const char **valuestart, >> + size_t *valuelen) >> +{ >> + const char *atom; >> + >> + if (!(skip_prefix(to_parse, candidate, &atom))) >> + return 0; >> + if (valuestart) { > > As far as I saw, no callers pass NULL to valuestart. Getting rid of > this if() statement and always entering its body would clarify what > is going on, I think. Specifically, ... >> + if (*atom == '=') { >> + *valuestart = atom + 1; >> + *valuelen = strcspn(*valuestart, ",\0"); >> + atom = *valuestart + *valuelen; >> + } else { >> + if (*atom != ',' && *atom != '\0') >> + return 0; >> + *valuestart = NULL; >> + *valuelen = 0; >> + } >> + } >> + if (*atom == ',') { >> + *end = atom + 1; >> + return 1; >> + } >> + if (*atom == '\0') { >> + *end = atom; >> + return 1; >> + } >> + return 0; >> +} ... I think the body of the function would become easier to read if written like so: if (!skip_prefix(to_parse, candidate, &atom)) return 0; /* definitely not "candidate" */ if (*atom == '=') { /* we just saw "candidate=" */ *valuestart = atom + 1; atom = strchrnul(*valuestart, ','); *valuelen = atom - *valuestart; } else if (*atom != ',' && *atom != '\0') { /* key begins with "candidate" but has more chars */ return 0; } else { /* just "candidate" without "=val" */ *valuestart = NULL; *valuelen = 0; } /* atom points at either the ',' or NUL after this key[=val] */ if (*atom == ',') atom++; else if (*atom) BUG("should not happen"); *end = atom; return 1; as it is clear that *valuestart, *valuelen, and *end are not touched when the function returns 0 and they are all filled when the function returns 1. Also, avoid passing ",\0" to strcspn(); its effect is exactly the same as passing ",", and at that point you are better off using strchnul(). Thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-19 23:23 ` Junio C Hamano 2023-07-20 5:21 ` Junio C Hamano @ 2023-07-20 16:52 ` Kousik Sanagavarapu 2023-07-20 17:59 ` Junio C Hamano 2023-07-20 17:42 ` Glen Choo 2 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-20 16:52 UTC (permalink / raw) To: Junio C Hamano Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh, Glen Choo On Wed, Jul 19, 2023 at 04:23:15PM -0700, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > New helper functions that do not have any caller and no > documentation to explain how they are supposed to be called > (i.e. the expectation on the callers---what values they need to feed > as parameters when they call these helpers, and the expectation by > the callers---what they expect to get out of the helpers once they > return) makes it impossible to evaluate if they are any good [*]. > > Side note. Those of you who are keen to add unit tests to > the system (Cc:ed) , do you think a patch line this one that > adds a new helper function to the system, would benefit from > being able to add a few unit tests for these otherwise > unused helper functions? > > The calls to the new functions that the unit test framework > would make should serve as a good piece of interface > documentation, showing what the callers are supposed to pass > and what they expect, I guess. > > So whatever framework we choose, it should allow adding a > test or two to this patch easily, without being too > intrusive. Would that be a good and concrete evaluation > criterion? > > Anyway, because of that, I had to read [2/2] first and then come > back here to review this one. > > The following is my attempt to write down the contract between the > callers and this new helper function---please give something like > that to the final version. The the example below is there just to > illustrate the level of information that would be desired to help > future readers and programmers. Do not take the contents as-written > as truth---I may have (deliberately) mixed in incorrect descriptions > ;-). I'll spot them---if there are any ;). > > /* > * The string "to_parse" is expected to be a comma-separated list > * of "key" or "key=val". If your atom allows "key1" and "key2" > * (possibly with their values) as options, make two calls to this > * funtion, passing "key1" in candiate and then passing "key2" in > * candidate. > * > * The function Returns true ONLY when the to_parse string begins > * with the candidate key, possibly followed by its value (valueless > * key-only entries are allowed in the comman-separated list). > * Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and > * the function returns false. > * > * *valuestart will point at the byte after '=' (i.e. the beginning > * of the value), and the number of bytes in the value will be set > * to *valuelen. > * A key-only entry results in *valuestart set to NULL and *valuelen > * set to 0. > * *end will point at the next key[=val] in the comma-separated list > * or NULL when the list ran out. > */ > > > +static int match_atom_arg_value(const char *to_parse, const char *candidate, > > + const char **end, const char **valuestart, > > + size_t *valuelen) > > +{ > > + const char *atom; > > + > > + if (!(skip_prefix(to_parse, candidate, &atom))) > > + return 0; > > + if (valuestart) { > > As far as I saw, no callers pass NULL to valuestart. Getting rid of > this if() statement and always entering its body would clarify what > is going on, I think. > > > + if (*atom == '=') { > > + *valuestart = atom + 1; > > + *valuelen = strcspn(*valuestart, ",\0"); > > + atom = *valuestart + *valuelen; > > + } else { > > + if (*atom != ',' && *atom != '\0') > > + return 0; > > + *valuestart = NULL; > > + *valuelen = 0; > > + } > > + } > > + if (*atom == ',') { > > + *end = atom + 1; > > + return 1; > > + } > > + if (*atom == '\0') { > > + *end = atom; > > + return 1; > > + } > > + return 0; > > +} > > /* > * Write something similar to document the contract between the caller > * and this function here. > */ > > +static int match_atom_bool_arg(const char *to_parse, const char *candidate, > > + const char **end, int *val) > > +{ I'll make these changes in the re-rolled version. I've also read your reply to this email with the changes in `match_atom_arg_value()`. I'll add them too. Going off in a tangent here---In yesterday's review club which discussed the %(decorate:<options>) patch[1], Glen suggested the possibility of having a single kind of a framework (used this word very loosely here) for parsing these multiple options since we are beginning to see them so often (might also help new formats which maybe added in the future). The fact that this wasn't done already says something about its difficulty as Jacob mentioned yesterday. The difficulty being we don't exactly know which options to parse as they differ from format to format. Christian, Hariom and I had a similar discussion about refactoring these helper functions that are already there in pretty (`match_placeholder_*()`) so that they can be used here. One example of this usage of functions from pretty is done when making ref-filter support trailers. [1]: https://lore.kernel.org/git/20230715160730.4046-1-andy.koppe@gmail.com/ Thanks ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-20 16:52 ` Kousik Sanagavarapu @ 2023-07-20 17:59 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-20 17:59 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh, Glen Choo Kousik Sanagavarapu <five231003@gmail.com> writes: > Going off in a tangent here---In yesterday's review club which discussed > the %(decorate:<options>) patch[1], Glen suggested the possibility of > having a single kind of a framework (used this word very loosely here) > for parsing these multiple options since we are beginning to see them so > often (might also help new formats which maybe added in the future). The > fact that this wasn't done already says something about its difficulty > as Jacob mentioned yesterday. The difficulty being we don't exactly know > which options to parse as they differ from format to format. Yes, while writing the sample in-code documentation for the match_atom_arg_value() function, I found its interface to force the callers to do "parse if it is 'tags'; else parse if it is 'abbrev'; and so on" hard to use. I wondered if the primitive to parse these should be modeled after config.c:parse_config_key(), i.e. the helper function takes the input and returns the split point and lengths of the components it finds in the return parameter. As the contract between the caller and the callee is that the caller passes the beginning of "key1[=val1],key2[=val2],...", the interface may look like int parse_cskv(const char *to_parse, const char **key, size_t *keylen, const char **val, size_t *vallen, const char **end); and would be used like so: const char *cskv = "key1,key2=val2"; const char *key, *val; size_t keylen, vallen; while (parse_cskv(cskv, &key, &keylen, &val, &vallen, &cskv) { if (!val) printf("valueless key '%.*s'\n", (int)keylen, key); else printf("key-value pair '%.*s=%.*s'\n", (int)keylen, key, (int)vallen, val); } if (*cskv) fprintf(stderr, "error - trailing garbage seen '%s'\n", cskv); The helper's contract to the caller may look like this: - It expects the string to be "key" or "key=val" followed by ',' or '\0' (the latter ends the input, i.e. the last element of comma separated list). The out variables key, val, keylen, vallen are used to convey to the caller where key and val are found and how long they are. - If it is a valueless "key", the out variable val is set to NULL. - The out variable end is updated to point at one byte after the element that has just been parsed. The need for the caller to check against the list of keys it knows about in the loop still exists, but the parser may become simpler that way. I dunno. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-19 23:23 ` Junio C Hamano 2023-07-20 5:21 ` Junio C Hamano 2023-07-20 16:52 ` Kousik Sanagavarapu @ 2023-07-20 17:42 ` Glen Choo 2023-07-20 20:30 ` Junio C Hamano 2 siblings, 1 reply; 38+ messages in thread From: Glen Choo @ 2023-07-20 17:42 UTC (permalink / raw) To: Junio C Hamano, Kousik Sanagavarapu Cc: git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh Junio C Hamano <gitster@pobox.com> writes: > New helper functions that do not have any caller and no > documentation to explain how they are supposed to be called > (i.e. the expectation on the callers---what values they need to feed > as parameters when they call these helpers, and the expectation by > the callers---what they expect to get out of the helpers once they > return) makes it impossible to evaluate if they are any good [*]. Agreed. > Side note. Those of you who are keen to add unit tests to > the system (Cc:ed) , do you think a patch line this one that > adds a new helper function to the system, would benefit from > being able to add a few unit tests for these otherwise > unused helper functions? Absolutely. As a rule, we should strive to test all of our changes as they are introduced. With our current shell-based testing, this means that we have to add callers (either via a builtin or test-helper), but IMO a unit test framework would serve this purpose even better. > The calls to the new functions that the unit test framework > would make should serve as a good piece of interface > documentation, showing what the callers are supposed to pass > and what they expect, I guess. Agreed, and as documentation, unit tests can be easier to read, since they can include only the relevant details. > So whatever framework we choose, it should allow adding a > test or two to this patch easily, without being too > intrusive. Would that be a good and concrete evaluation > criterion? Perhaps, but the biggest blocker to adding a unit tests is whether the source file itself is amenable to being unit tested (e.g. does it depend on global state? does it compile easily?). Once that is in place, I can't imagine that there would be a sensible unit test framework that doesn't make it easy to add tests to a patch like this. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-20 17:42 ` Glen Choo @ 2023-07-20 20:30 ` Junio C Hamano 2023-07-21 18:26 ` Glen Choo 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-20 20:30 UTC (permalink / raw) To: Glen Choo Cc: Kousik Sanagavarapu, git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh Glen Choo <chooglen@google.com> writes: >> So whatever framework we choose, it should allow adding a >> test or two to this patch easily, without being too >> intrusive. Would that be a good and concrete evaluation >> criterion? > > Perhaps, but the biggest blocker to adding a unit tests is whether the > source file itself is amenable to being unit tested (e.g. does it depend > on global state? does it compile easily?). Perhaps. Now would this particular example, the change to ref-filter.c file, be a reasonable guinea-pig test case for candidate test frameworks to add tests for these two helper functions? They are pretty-much plain vanilla string manipulation functions that does not depend too many things that are specific to Git. They may use helpers we wrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(), but they shouldn't depend on the program start-up sequence, discovering repositories, installing at-exit handers, and other stuff. It was why I wondered if it can be used as a good evaluation criterion---if a test framework cannot easily add tests while this patch was being proposed in a non-intrusive way to demonstrate how these two functions are supposed to work and to protect their implementations from future breakage, it would not be all that useful, I would imagine. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions 2023-07-20 20:30 ` Junio C Hamano @ 2023-07-21 18:26 ` Glen Choo 0 siblings, 0 replies; 38+ messages in thread From: Glen Choo @ 2023-07-21 18:26 UTC (permalink / raw) To: Junio C Hamano Cc: Kousik Sanagavarapu, git, Christian Couder, Hariom Verma, Josh Steadmon, Siddharth Singh Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >>> So whatever framework we choose, it should allow adding a >>> test or two to this patch easily, without being too >>> intrusive. Would that be a good and concrete evaluation >>> criterion? >> >> Perhaps, but the biggest blocker to adding a unit tests is whether the >> source file itself is amenable to being unit tested (e.g. does it depend >> on global state? does it compile easily?). > > Now would this particular example, the change to ref-filter.c file, > be a reasonable guinea-pig test case for candidate test frameworks > to add tests for these two helper functions? They are pretty-much > plain vanilla string manipulation functions that does not depend too > many things that are specific to Git. Ah, yes. This would be close-to-ideal candidate then. > They may use helpers we > wrote, i.e. xstrndup(), skip_prefix(), and git_parse_maybe_bool(), > but they shouldn't depend on the program start-up sequence, > discovering repositories, installing at-exit handers, and other > stuff. It was why I wondered if it can be used as a good evaluation > criterion---if a test framework cannot easily add tests while this > patch was being proposed in a non-intrusive way to demonstrate how > these two functions are supposed to work and to protect their > implementations from future breakage, it would not be all that > useful, I would imagine. The current thinking among Googlers is that we won't remove the helpers from library code. Git will either provide them, e.g. via Calvin's git-std-lib RFC [1], or we will provide ways for callers to bring their own implementation (like trace2 or exit, since it doesn't necessarily make sense to use Git's implementation). So yes, the test framework should be able to support this sort of compilation pattern. I'm not sure how much test frameworks differ in this regard, maybe Josh has some insight here. [1] https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 2/2] ref-filter: add new "describe" atom 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-19 16:15 ` Kousik Sanagavarapu 2023-07-19 22:56 ` Junio C Hamano 2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu 3 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-19 16:15 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Christian Couder, Hariom Verma, Kousik Sanagavarapu Duplicate the logic of %(describe) and friends from pretty to ref-filter. In the future, this change helps in unifying both the formats as ref-filter will be able to do everything that pretty is doing and we can have a single interface. The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/git-for-each-ref.txt | 23 +++++ ref-filter.c | 130 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++ 3 files changed, 267 insertions(+) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 2e0318770b..395daf1b22 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -258,6 +258,29 @@ ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. +describe[:options]:: Human-readable name, like + link-git:git-describe[1]; empty string for + undescribable commits. The `describe` string may be + followed by a colon and zero or more comma-separated + options. Descriptions can be inconsistent when tags + are added or removed at the same time. ++ +-- +tags=<bool-value>;; Instead of only considering annotated tags, consider + lightweight tags as well; see the corresponding option + in linkgit:git-describe[1] for details. +abbrev=<number>;; Use at least <number> hexadecimal digits; see + the corresponding option in linkgit:git-describe[1] + for details. +match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding + option in linkgit:git-describe[1] for details. +exclude=<pattern>;; Do not consider tags matching the given `glob(7)` + pattern, excluding the "refs/tags/" prefix; see the + corresponding option in linkgit:git-describe[1] for + details. +-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/ref-filter.c b/ref-filter.c index f64437e781..43316643be 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1,9 +1,11 @@ #include "git-compat-util.h" #include "environment.h" #include "gettext.h" +#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" +#include "run-command.h" #include "refs.h" #include "wildmatch.h" #include "object-name.h" @@ -145,6 +147,7 @@ enum atom_type { ATOM_TAGGERDATE, ATOM_CREATOR, ATOM_CREATORDATE, + ATOM_DESCRIBE, ATOM_SUBJECT, ATOM_BODY, ATOM_TRAILERS, @@ -219,6 +222,7 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + const char **describe_args; struct refname_atom refname; char *head; } u; @@ -554,6 +558,91 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato return 0; } +static int describe_atom_option_parser(struct strvec *args, const char **arg, + struct strbuf *err) +{ + const char *argval; + size_t arglen = 0; + int optval = 0; + + if (match_atom_bool_arg(*arg, "tags", arg, &optval)) { + if (!optval) + strvec_push(args, "--no-tags"); + else + strvec_push(args, "--tags"); + return 1; + } + + if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) { + char *endptr; + + if (!arglen) + return strbuf_addf_ret(err, -1, + _("argument expected for %s"), + "describe:abbrev"); + if (strtol(argval, &endptr, 10) < 0) + return strbuf_addf_ret(err, -1, + _("positive value expected %s=%s"), + "describe:abbrev", argval); + if (endptr - argval != arglen) + return strbuf_addf_ret(err, -1, + _("cannot fully parse %s=%s"), + "describe:abbrev", argval); + + strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:match"); + + strvec_pushf(args, "--match=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:exclude"); + + strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval); + return 1; + } + + return 0; +} + +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + struct strvec args = STRVEC_INIT; + + for (;;) { + int found = 0; + const char *bad_arg = NULL; + + if (!arg || !*arg) + break; + + bad_arg = arg; + found = describe_atom_option_parser(&args, &arg, err); + if (found < 0) + return found; + if (!found) { + if (bad_arg && *bad_arg) + return err_bad_arg(err, "describe", bad_arg); + break; + } + } + atom->u.describe_args = strvec_detach(&args); + return 0; +} + static int raw_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) @@ -756,6 +845,7 @@ static struct { [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, @@ -1662,6 +1752,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } } +static void grab_describe_values(struct atom_value *val, int deref, + struct object *obj) +{ + struct commit *commit = (struct commit *)obj; + int i; + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + enum atom_type type = atom->atom_type; + const char *name = atom->name; + struct atom_value *v = &val[i]; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + + if (type != ATOM_DESCRIBE) + continue; + + if (!!deref != (*name == '*')) + continue; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); + strvec_pushv(&cmd.args, atom->u.describe_args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); + v->s = xstrdup(""); + continue; + } + strbuf_rtrim(&out); + v->s = strbuf_detach(&out, NULL); + + strbuf_release(&err); + } +} + /* See grab_values */ static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data) { @@ -1771,6 +1899,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_tag_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("tagger", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); @@ -1778,6 +1907,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); grab_signature(val, deref, obj); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: /* grab_tree_values(val, deref, obj, buf, sz); */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 6e6ec852b5..4bbba76874 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -562,6 +562,120 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' +test_expect_success 'setup for describe atom tests' ' + git init describe-repo && + ( + cd describe-repo && + + test_commit --no-tag one && + git tag tagone && + + test_commit --no-tag two && + git tag -a -m "tag two" tagtwo + ) +' + +test_expect_success 'describe atom vs git describe' ' + ( + cd describe-repo && + + git for-each-ref --format="%(objectname)" \ + refs/tags/ >obj && + while read hash + do + if desc=$(git describe $hash) + then + : >expect-contains-good + else + : >expect-contains-bad + fi && + echo "$hash $desc" || return 1 + done <obj >expect && + test_path_exists expect-contains-good && + test_path_exists expect-contains-bad && + + git for-each-ref --format="%(objectname) %(describe)" \ + refs/tags/ >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + ) +' + +test_expect_success 'describe:tags vs describe --tags' ' + ( + cd describe-repo && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' + ( + cd describe-repo && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + test tagname = $(cat actual) + ) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' + ( + cd describe-repo && + git tag -a -m "tag foo" tag-foo && + git describe --match "*-foo" >expect && + git for-each-ref --format="%(describe:match="*-foo")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' + ( + cd describe-repo && + git tag -a -m "tag bar" tag-bar && + git describe --exclude "*-bar" >expect && + git for-each-ref --format="%(describe:exclude="*-bar")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'deref with describe atom' ' + ( + cd describe-repo && + cat >expect <<-\EOF && + + tagname + tagname + tagname + + tagtwo + EOF + git for-each-ref --format="%(*describe)" >actual && + test_cmp expect actual + ) +' + cat >expected <<\EOF heads/main tags/main -- 2.41.0.378.g42703afc1f.dirty ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 2/2] ref-filter: add new "describe" atom 2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-19 22:56 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-19 22:56 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > Duplicate the logic of %(describe) and friends from pretty to > ref-filter. In the future, this change helps in unifying both the > formats as ref-filter will be able to do everything that pretty is doing > and we can have a single interface. > > The new atom "describe" and its friends are equivalent to the existing > pretty formats with the same name. > > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: Hariom Verma <hariom18599@gmail.com> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > --- > Documentation/git-for-each-ref.txt | 23 +++++ > ref-filter.c | 130 +++++++++++++++++++++++++++++ > t/t6300-for-each-ref.sh | 114 +++++++++++++++++++++++++ > 3 files changed, 267 insertions(+) > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 2e0318770b..395daf1b22 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -258,6 +258,29 @@ ahead-behind:<committish>:: > commits ahead and behind, respectively, when comparing the output > ref to the `<committish>` specified in the format. > > +describe[:options]:: Human-readable name, like > + link-git:git-describe[1]; empty string for > + undescribable commits. The `describe` string may be > + followed by a colon and zero or more comma-separated > + options. Why do these new items formatted so differently from the previous ones? By indenting the lines so deeply you are forcing yourself to wrap these lines many times. How about imitating the previous entry for ahead-behind and writing this like so: describe[:<options>]:: A human-readable name, like linkgit:git-describe[1]; empty string is given for an undescribable commit. ... By the way, there is a typo "link-git" above that needs to be corrected. It is curious that we support "describe:" (i.e. having no options, but colon is still present). It may not be wrong per se, but it looks strange. "may be followed by a colon and one or more comma-separated options" would be more intuitive (I haven't seen the implementation yet, so if we go that route, the implementation may also need to be updated). > .... Descriptions can be inconsistent when tags > + are added or removed at the same time. "at the same time" meaning "while the description are being computed"? I think this was copied from 273c9901 (pretty: document multiple %(describe) being inconsistent, 2021-02-28) where the pretty placeholder for "git log" and friends are described, and the implementation used there go one formatting element at a time, unlike for-each-ref that can compute a description for a given ref just once in populate_value() and reuse the same atom number of times in the format, each instance giving exactly the same value. So I am not sure if the "can be inconsistent" disclaimer applies to the %(describe) on this side the same way. Are you sure? As %(describe) is fairly expensive to compute, if the format string wants two, e.g. --format="%(refname) %(describe) %(describe)", there should be some effort to make these two share the same used_atom(), so that there will be only one "git describe" invocation from populate_value() that lets get_ref_atom_value() reuse that result of a single invocation to fill the two placeholder. > @@ -219,6 +222,7 @@ static struct used_atom { > enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, > S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; > } signature; > + const char **describe_args; > struct refname_atom refname; > char *head; > } u; Nice and simple ;-). > +static int describe_atom_option_parser(struct strvec *args, const char **arg, > + struct strbuf *err) > +{ > + const char *argval; > + size_t arglen = 0; > + int optval = 0; > + > + if (match_atom_bool_arg(*arg, "tags", arg, &optval)) { > + if (!optval) > + strvec_push(args, "--no-tags"); > + else > + strvec_push(args, "--tags"); > + return 1; > + } OK. One thing that I hate about the split of this series into two steps is that [1/2] has to be read without knowing what the expected use of those two helper functions are. It was especially bad as the functions lacked any documentation on how they are supposed to be called. Now, if we go back to the implementation of match_atom_bool_arg(), it first called match_atom_arg_value(), which stripped the given key ("tags" in this case) from the argument being parsed, and allowed '=' (i.e. followed by a val), ',' (i.e. no val, but more "key[=val]" to follow), or '\0' (i.e. end of the argument string). Anything else after the matched key meant that the key did not match (e.g. the arg had "tagsabcd", which should not match "tag"). And it stored the byte position after '=' if the key was terminated with '=', or NULL otherwise, to signal where the optional value starts. match_atom_bool_arg() uses this and correctly translates a key without the optional [=val] part into "true". So the above is given, after the caller skips "%(describe:", things like "tags,...", "tags=no,...", "tags=yes,..." (or replace ",..." with a NUL for the final option), and chooses between --no-tags and --tags. Sounds good. > + ... > + if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) { > + if (!arglen) > + return strbuf_addf_ret(err, -1, > + _("value expected %s="), > + "describe:exclude"); > + > + strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval); > + return 1; > + } I would have expected that these become if/else if/.../else cascade, i.e. if (is that "tags"?) { } else if (is that "abbrev"?) { ... } else return 0; /* nothing matched */ return 1; but I do not mind the above. Each "block" that matches and handles one key looks more indenendent the way the patch was written, which may be a good thing. > + return 0; > +} > + > +static int describe_atom_parser(struct ref_format *format UNUSED, > + struct used_atom *atom, > + const char *arg, struct strbuf *err) > +{ > + struct strvec args = STRVEC_INIT; OK, parse_ref_fitler_atom() saw "%(describe", possibly followed by a colon and zero or more comma-separated key[=val], and the location after ':' (or NULL) is given to arg. Specifically, %(describe) and %(describe:) both pass NULL in arg. > + for (;;) { > + int found = 0; > + const char *bad_arg = NULL; > + > + if (!arg || !*arg) > + break; And we stop when there is no more key[=val]. > + bad_arg = arg; > + found = describe_atom_option_parser(&args, &arg, err); This one moves arg forward and arranges the next key[=val] to be seen in the next iteration of this loop. Makes sense. In the remainder of the code changes, I saw nothing strange. Quite well made. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-20 22:52 ` Junio C Hamano 2023-07-20 23:10 ` Junio C Hamano 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu 3 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-20 22:52 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > PATCH 1/2 - Left unchanged expect for small changes in the commit > message for more clarity. > > PATCH 2/2 - We now parse the arguments in a seperate function > `describe_atom_option_parser()` call this in > `describe_atom_parser()` instead to populate > `atom->u.describe_args`. This splitting of the function > helps err at the right places. This topic may be getting rerolled but from the CI logs, comparing * https://github.com/git/git/actions/runs/5603242871 (seen at 77ba682) that passes the tests * https://github.com/git/git/actions/runs/5605480104 (seen at 29f0316) that breaks linux-gcc (ubuntu-20.04) at t6300 [*] output from "git shortlog --no-merges 77ba682..29f0316" [*] makes us suspect that this topic may be the culprit of the recent breakage. The linux-gcc job is where we force the initial branch name to be 'main' and not 'master', so if your tests assume that the initial & primary branch name is 'master', that may be something you need to fix. Thanks. [Reference] * https://github.com/git/git/actions/runs/5605480104/job/15186229680 * git shortlog --no-merges 77ba682..29f0316 Alex Henrie (1): sequencer: finish parsing the todo list despite an invalid first line Beat Bolli (1): trace2: fix a comment Junio C Hamano (4): short help: allow multi-line opthelp remote: simplify "remote add --tags" help text short help: allow a gap smaller than USAGE_GAP ### Kousik Sanagavarapu (2): ref-filter: add multiple-option parsing functions ref-filter: add new "describe" atom ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom 2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano @ 2023-07-20 23:10 ` Junio C Hamano 2023-07-21 4:17 ` Kousik Sanagavarapu 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-20 23:10 UTC (permalink / raw) To: Kousik Sanagavarapu; +Cc: git, Christian Couder, Hariom Verma Junio C Hamano <gitster@pobox.com> writes: > The linux-gcc job is where we force the initial branch name to be > 'main' and not 'master', so if your tests assume that the initial & > primary branch name is 'master', that may be something you need to > fix. Perhaps something along the line of the attached patch? The primary test repository t6300 uses is aware of the "problem" where the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to 'main' and hacks it around by using git branch -M main as one of the first things it does, to _force_ the primary branch name always to 'main', whether the tester's environment forces "git" to start with 'main' or 'master', and existing tests in the script relies on 'main' being the primary branch. But your tests are done in a repository newly created with your own "git init", so depending on the tester's environment, the primary branch may be 'master' or 'main'. The way your new tests are written, however, things will fail if "refs/heads/master" is not the primary branch. t/t6300-for-each-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh index 4bbba76874..489f4d9186 100755 --- c/t/t6300-for-each-ref.sh +++ w/t/t6300-for-each-ref.sh @@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' ' ' test_expect_success 'setup for describe atom tests' ' - git init describe-repo && + git init -b master describe-repo && ( cd describe-repo && ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/2] Add new "describe" atom 2023-07-20 23:10 ` Junio C Hamano @ 2023-07-21 4:17 ` Kousik Sanagavarapu 0 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-21 4:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Christian Couder, Hariom Verma On Thu, Jul 20, 2023 at 04:10:09PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > The linux-gcc job is where we force the initial branch name to be > > 'main' and not 'master', so if your tests assume that the initial & > > primary branch name is 'master', that may be something you need to > > fix. > > Perhaps something along the line of the attached patch? > > The primary test repository t6300 uses is aware of the "problem" > where the tester may set GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > to 'main' and hacks it around by using > > git branch -M main > > as one of the first things it does, to _force_ the primary branch > name always to 'main', whether the tester's environment forces "git" > to start with 'main' or 'master', and existing tests in the script > relies on 'main' being the primary branch. > > But your tests are done in a repository newly created with your own > "git init", so depending on the tester's environment, the primary > branch may be 'master' or 'main'. The way your new tests are > written, however, things will fail if "refs/heads/master" is not the > primary branch. > I see. I looked at the trash directory by doing -v -i -d when running these describe tests and was under the impression that it is always master.I guess I should have had a bigger view of things. > t/t6300-for-each-ref.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh > index 4bbba76874..489f4d9186 100755 > --- c/t/t6300-for-each-ref.sh > +++ w/t/t6300-for-each-ref.sh > @@ -563,7 +563,7 @@ test_expect_success 'color.ui=always does not override tty check' ' > ' > > test_expect_success 'setup for describe atom tests' ' > - git init describe-repo && > + git init -b master describe-repo && > ( > cd describe-repo && I'll add this to the re-rolled version. Thanks ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 0/2] Add new "describe" atom 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu ` (2 preceding siblings ...) 2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano @ 2023-07-23 16:19 ` Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu ` (2 more replies) 3 siblings, 3 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu Hi, Thanks for the review on the previous version. PATCH 1/2 - Added comments to explain what the helper functions do and also fixed a nit where config.h should be included here and not the subsequent commit. PATCH 2/2 - Changed the formatting on Documentation to follow what was in the file. Also, the statement "Descriptions can be insconsistent when tags are added or removed at the same time" is not true in ref-filter's case as, $ time git for-each-ref --format="%(describe)" refs/ real 0m19.936s user 0m11.488s sys 0m7.915s $ time git for-each-ref --format="%(describe) %(describe)" refs/ real 0m19.502s user 0m11.653s sys 0m7.623s I also added a test to check that we err on bad describe args. Kousik Sanagavarapu (2): ref-filter: add multiple-option parsing functions ref-filter: add new "describe" atom Documentation/git-for-each-ref.txt | 23 +++ ref-filter.c | 230 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 138 +++++++++++++++++ 3 files changed, 391 insertions(+) Range-diff against v3: 1: 08f3be1631 ! 1: 2914bd58ec ref-filter: add multiple-option parsing functions @@ Commit message are going to add a new %(describe) atom in a subsequent commit where we parse options like tags=<bool-value> or match=<pattern> given to it. + Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> ## ref-filter.c ## +@@ + #include "git-compat-util.h" + #include "environment.h" + #include "gettext.h" ++#include "config.h" + #include "gpg-interface.h" + #include "hex.h" + #include "parse-options.h" @@ ref-filter.c: static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg) return -1; } ++/* ++ * Parse option of name "candidate" in the option string "to_parse" of ++ * the form ++ * ++ * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..." ++ * ++ * The remaining part of "to_parse" is stored in "end" (if we are ++ * parsing the last candidate, then this is NULL) and the value of ++ * the candidate is stored in "valuestart" and its length in "valuelen", ++ * that is the portion after "=". Since it is possible for a "candidate" ++ * to not have a value, in such cases, "valuestart" is set to point to ++ * NULL and "valuelen" to 0. ++ * ++ * The function returns 1 on success. It returns 0 if we don't find ++ * "candidate" in "to_parse" or we find "candidate" but it is followed ++ * by more chars (for example, "candidatefoo"), that is, we don't find ++ * an exact match. ++ * ++ * This function only does the above for one "candidate" at a time. So ++ * it has to be called each time trying to parse a "candidate" in the ++ * option string "to_parse". ++ */ +static int match_atom_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) +{ + const char *atom; + -+ if (!(skip_prefix(to_parse, candidate, &atom))) ++ if (!skip_prefix(to_parse, candidate, &atom)) ++ return 0; /* definitely not "candidate" */ ++ ++ if (*atom == '=') { ++ /* we just saw "candidate=" */ ++ *valuestart = atom + 1; ++ atom = strchrnul(*valuestart, ','); ++ *valuelen = atom - *valuestart; ++ } else if (*atom != ',' && *atom != '\0') { ++ /* key begins with "candidate" but has more chars */ + return 0; -+ if (valuestart) { -+ if (*atom == '=') { -+ *valuestart = atom + 1; -+ *valuelen = strcspn(*valuestart, ",\0"); -+ atom = *valuestart + *valuelen; -+ } else { -+ if (*atom != ',' && *atom != '\0') -+ return 0; -+ *valuestart = NULL; -+ *valuelen = 0; -+ } -+ } -+ if (*atom == ',') { -+ *end = atom + 1; -+ return 1; -+ } -+ if (*atom == '\0') { -+ *end = atom; -+ return 1; ++ } else { ++ /* just "candidate" without "=val" */ ++ *valuestart = NULL; ++ *valuelen = 0; + } -+ return 0; ++ ++ /* atom points at either the ',' or NUL after this key[=val] */ ++ if (*atom == ',') ++ atom++; ++ else if (*atom) ++ BUG("Why is *atom not NULL yet?"); ++ ++ *end = atom; ++ return 1; +} + ++/* ++ * Parse boolean option of name "candidate" in the option list "to_parse" ++ * of the form ++ * ++ * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..." ++ * ++ * The remaining part of "to_parse" is stored in "end" (if we are parsing ++ * the last candidate, then this is NULL) and the value (if given) is ++ * parsed and stored in "val", so "val" always points to either 0 or 1. ++ * If the value is not given, then "val" is set to point to 1. ++ * ++ * The boolean value is parsed using "git_parse_maybe_bool()", so the ++ * accepted values are ++ * ++ * to set true - "1", "yes", "true" ++ * to set false - "0", "no", "false" ++ * ++ * This function returns 1 on success. It returns 0 when we don't find ++ * an exact match for "candidate" or when the boolean value given is ++ * not valid. ++ */ +static int match_atom_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ 2: 742a79113c ! 2: 77a2a56520 ref-filter: add new "describe" atom @@ Commit message The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. + Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. -+describe[:options]:: Human-readable name, like -+ link-git:git-describe[1]; empty string for -+ undescribable commits. The `describe` string may be -+ followed by a colon and zero or more comma-separated -+ options. Descriptions can be inconsistent when tags -+ are added or removed at the same time. ++describe[:options]:: ++ A human-readable name, like linkgit:git-describe[1]; ++ empty string for undescribable commits. The `describe` string may ++ be followed by a colon and one or more comma-separated options. ++ +-- -+tags=<bool-value>;; Instead of only considering annotated tags, consider -+ lightweight tags as well; see the corresponding option -+ in linkgit:git-describe[1] for details. -+abbrev=<number>;; Use at least <number> hexadecimal digits; see -+ the corresponding option in linkgit:git-describe[1] -+ for details. -+match=<pattern>;; Only consider tags matching the given `glob(7)` pattern, -+ excluding the "refs/tags/" prefix; see the corresponding -+ option in linkgit:git-describe[1] for details. -+exclude=<pattern>;; Do not consider tags matching the given `glob(7)` -+ pattern, excluding the "refs/tags/" prefix; see the -+ corresponding option in linkgit:git-describe[1] for -+ details. ++tags=<bool-value>;; ++ Instead of only considering annotated tags, consider ++ lightweight tags as well; see the corresponding option in ++ linkgit:git-describe[1] for details. ++abbrev=<number>;; ++ Use at least <number> hexadecimal digits; see the corresponding ++ option in linkgit:git-describe[1] for details. ++match=<pattern>;; ++ Only consider tags matching the given `glob(7)` pattern, ++ excluding the "refs/tags/" prefix; see the corresponding option ++ in linkgit:git-describe[1] for details. ++exclude=<pattern>;; ++ Do not consider tags matching the given `glob(7)` pattern, ++ excluding the "refs/tags/" prefix; see the corresponding option ++ in linkgit:git-describe[1] for details. +-- + In addition to the above, for commit and tag objects, the header @@ Documentation/git-for-each-ref.txt: ahead-behind:<committish>:: ## ref-filter.c ## @@ - #include "git-compat-util.h" - #include "environment.h" - #include "gettext.h" -+#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" @@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct + + for (;;) { + int found = 0; -+ const char *bad_arg = NULL; ++ const char *bad_arg = arg; + + if (!arg || !*arg) + break; + -+ bad_arg = arg; + found = describe_atom_option_parser(&args, &arg, err); + if (found < 0) + return found; -+ if (!found) { -+ if (bad_arg && *bad_arg) -+ return err_bad_arg(err, "describe", bad_arg); -+ break; -+ } ++ if (!found) ++ return err_bad_arg(err, "describe", bad_arg); + } + atom->u.describe_args = strvec_detach(&args); + return 0; @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override ' +test_expect_success 'setup for describe atom tests' ' -+ git init describe-repo && ++ git init -b master describe-repo && + ( + cd describe-repo && + @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override + test_cmp expect actual + ) +' ++ ++test_expect_success 'err on bad describe atom arg' ' ++ ( ++ cd describe-repo && ++ ++ # The bad arg is the only arg passed to describe atom ++ cat >expect <<-\EOF && ++ fatal: unrecognized %(describe) argument: baz ++ EOF ++ ! git for-each-ref --format="%(describe:baz)" \ ++ refs/heads/master 2>actual && ++ test_cmp expect actual && ++ ++ # The bad arg is in the middle of the option string ++ # passed to the describe atom ++ cat >expect <<-\EOF && ++ fatal: unrecognized %(describe) argument: qux=1,abbrev=14 ++ EOF ++ ! git for-each-ref \ ++ --format="%(describe:tags,qux=1,abbrev=14)" \ ++ ref/heads/master 2>actual && ++ test_cmp expect actual ++ ) ++' + cat >expected <<\EOF heads/main -- 2.41.0.396.g9ab76b0018 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 1/2] ref-filter: add multiple-option parsing functions 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu @ 2023-07-23 16:19 ` Kousik Sanagavarapu 2023-07-24 17:29 ` Junio C Hamano 2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu 2 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu The functions match_placeholder_arg_value() match_placeholder_bool_arg() were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options with explicit value, 2019-01-29) to parse multiple options in an argument to --pretty. For example, git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" will output all the trailers matching the key and seperates them by a comma followed by a space per commit. Add similar functions, match_atom_arg_value() match_atom_bool_arg() in ref-filter. There is no atom yet that can use these functions in ref-filter, but we are going to add a new %(describe) atom in a subsequent commit where we parse options like tags=<bool-value> or match=<pattern> given to it. Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index 0f3df132b8..8d5f85e0a7 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1,6 +1,7 @@ #include "git-compat-util.h" #include "environment.h" #include "gettext.h" +#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" @@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg) return -1; } +/* + * Parse option of name "candidate" in the option string "to_parse" of + * the form + * + * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..." + * + * The remaining part of "to_parse" is stored in "end" (if we are + * parsing the last candidate, then this is NULL) and the value of + * the candidate is stored in "valuestart" and its length in "valuelen", + * that is the portion after "=". Since it is possible for a "candidate" + * to not have a value, in such cases, "valuestart" is set to point to + * NULL and "valuelen" to 0. + * + * The function returns 1 on success. It returns 0 if we don't find + * "candidate" in "to_parse" or we find "candidate" but it is followed + * by more chars (for example, "candidatefoo"), that is, we don't find + * an exact match. + * + * This function only does the above for one "candidate" at a time. So + * it has to be called each time trying to parse a "candidate" in the + * option string "to_parse". + */ +static int match_atom_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) +{ + const char *atom; + + if (!skip_prefix(to_parse, candidate, &atom)) + return 0; /* definitely not "candidate" */ + + if (*atom == '=') { + /* we just saw "candidate=" */ + *valuestart = atom + 1; + atom = strchrnul(*valuestart, ','); + *valuelen = atom - *valuestart; + } else if (*atom != ',' && *atom != '\0') { + /* key begins with "candidate" but has more chars */ + return 0; + } else { + /* just "candidate" without "=val" */ + *valuestart = NULL; + *valuelen = 0; + } + + /* atom points at either the ',' or NUL after this key[=val] */ + if (*atom == ',') + atom++; + else if (*atom) + BUG("Why is *atom not NULL yet?"); + + *end = atom; + return 1; +} + +/* + * Parse boolean option of name "candidate" in the option list "to_parse" + * of the form + * + * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..." + * + * The remaining part of "to_parse" is stored in "end" (if we are parsing + * the last candidate, then this is NULL) and the value (if given) is + * parsed and stored in "val", so "val" always points to either 0 or 1. + * If the value is not given, then "val" is set to point to 1. + * + * The boolean value is parsed using "git_parse_maybe_bool()", so the + * accepted values are + * + * to set true - "1", "yes", "true" + * to set false - "0", "no", "false" + * + * This function returns 1 on success. It returns 0 when we don't find + * an exact match for "candidate" or when the boolean value given is + * not valid. + */ +static int match_atom_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *argval; + char *strval; + size_t arglen; + int v; + + if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen)) + return 0; + + if (!argval) { + *val = 1; + return 1; + } + + strval = xstrndup(argval, arglen); + v = git_parse_maybe_bool(strval); + free(strval); + + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static int color_atom_parser(struct ref_format *format, struct used_atom *atom, const char *color_value, struct strbuf *err) { -- 2.41.0.396.g9ab76b0018 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions 2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-24 17:29 ` Junio C Hamano 2023-07-24 18:12 ` Kousik Sanagavarapu 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-24 17:29 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > The functions > > match_placeholder_arg_value() > match_placeholder_bool_arg() > > were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options > with explicit value, 2019-01-29) to parse multiple options in an > argument to --pretty. For example, > > git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" > > will output all the trailers matching the key and seperates them by > a comma followed by a space per commit. > > Add similar functions, > > match_atom_arg_value() > match_atom_bool_arg() > > in ref-filter. What are their similarities, and in what way are they different? If they are similar enough, is it reasonable to allow these two pairs of helpers to share code (the best case would be we can just call the existing ones, possibly changing their names to more suitable ones that fit their now-more-general-purpose nature better)? > There is no atom yet that can use these functions in ref-filter, but we > are going to add a new %(describe) atom in a subsequent commit where we > parse options like tags=<bool-value> or match=<pattern> given to it. > > Helped-by: Junio C Hamano <gitster@pobox.com> > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: Hariom Verma <hariom18599@gmail.com> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > --- > ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 105 insertions(+) Asking just out of curiousity, all patches from you seem to have "Mentored-by" naming your mentors, but how deeply involved are they in each patch you send out? Is it like you first ask them to review and only after addressing the issues their reviews raise, you are sending the polished patches to the list? Or are they not deeply involved in the code but offering suggestions on the design (I am curious what their reactions were on your design decision to add the two helper functions)? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions 2023-07-24 17:29 ` Junio C Hamano @ 2023-07-24 18:12 ` Kousik Sanagavarapu 2023-07-24 20:39 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-24 18:12 UTC (permalink / raw) To: Junio C Hamano Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma On Mon, Jul 24, 2023 at 10:29:52AM -0700, Junio C Hamano wrote: > Kousik Sanagavarapu <five231003@gmail.com> writes: > > > The functions > > > > match_placeholder_arg_value() > > match_placeholder_bool_arg() > > > > were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options > > with explicit value, 2019-01-29) to parse multiple options in an > > argument to --pretty. For example, > > > > git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" > > > > will output all the trailers matching the key and seperates them by > > a comma followed by a space per commit. > > > > Add similar functions, > > > > match_atom_arg_value() > > match_atom_bool_arg() > > > > in ref-filter. > > What are their similarities, and in what way are they different? If > they are similar enough, is it reasonable to allow these two pairs > of helpers to share code (the best case would be we can just call > the existing ones, possibly changing their names to more suitable > ones that fit their now-more-general-purpose nature better)? What do you mean by "share code"? They are similar in their functionality, that is parsing the option and grabbing the value (if the option has a value, otherwise we do what we did here). The difference is the way we do such a parsing. In pretty, we directly skip_prefix() the placeholder. So we check for ')' to see if we have reached the end of "to_parse". In ref-filter (the current patches), we deal directly with the options ("arg" here), that is we can't do a check for ')' to see if we have exhausted our option list. So we can't really use the same functions, but there is the possiblity that we can modify them to be used here too. So the difference is mainly just how we send "to_parse" and how we want it parsed. > > There is no atom yet that can use these functions in ref-filter, but we > > are going to add a new %(describe) atom in a subsequent commit where we > > parse options like tags=<bool-value> or match=<pattern> given to it. > > > > Helped-by: Junio C Hamano <gitster@pobox.com> > > Mentored-by: Christian Couder <christian.couder@gmail.com> > > Mentored-by: Hariom Verma <hariom18599@gmail.com> > > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > > --- > > ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 105 insertions(+) > > Asking just out of curiousity, all patches from you seem to have > "Mentored-by" naming your mentors, but how deeply involved are they > in each patch you send out? Is it like you first ask them to review > and only after addressing the issues their reviews raise, you are > sending the polished patches to the list? Or are they not deeply > involved in the code but offering suggestions on the design Both actually, the code and the design. I send them the commits which I push to my fork and they take a look on the code as well as the design and offer suggestions on how both can be improved or re-did. > (I am > curious what their reactions were on your design decision to > add the two helper functions)? They suggested doing something similar to what you suggested above but it is kind of on hold (also because of how we changed the implementation of "match_atom_arg_value()"). Now that you bring it up, should this patch be reworked? Thanks ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions 2023-07-24 18:12 ` Kousik Sanagavarapu @ 2023-07-24 20:39 ` Junio C Hamano 2023-07-25 19:27 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Junio C Hamano @ 2023-07-24 20:39 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > What do you mean by "share code"? > > They are similar in their functionality, that is parsing the option and > grabbing the value (if the option has a value, otherwise we do what we > did here). The difference is the way we do such a parsing. > > In pretty, we directly skip_prefix() the placeholder. So we check for ')' > to see if we have reached the end of "to_parse". > > In ref-filter (the current patches), we deal directly with the options > ("arg" here), that is we can't do a check for ')' to see if we have > exhausted our option list. So we can't really use the same functions, but > there is the possiblity that we can modify them to be used here too. That is the kind of "sharing" to reduce repetition I had in mind. I haven't checked the callers, but another way would be to update the caller of for-each-ref's side to match the calling convention of how pretty calls the parser, wouldn't it? After all, they parse the same "%(token:key=val,key=val,...)" so...? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v4 1/2] ref-filter: add multiple-option parsing functions 2023-07-24 20:39 ` Junio C Hamano @ 2023-07-25 19:27 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-25 19:27 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma Junio C Hamano <gitster@pobox.com> writes: > Kousik Sanagavarapu <five231003@gmail.com> writes: > >> What do you mean by "share code"? >> >> They are similar in their functionality, that is parsing the option and >> grabbing the value (if the option has a value, otherwise we do what we >> did here). The difference is the way we do such a parsing. >> >> In pretty, we directly skip_prefix() the placeholder. So we check for ')' >> to see if we have reached the end of "to_parse". >> >> In ref-filter (the current patches), we deal directly with the options >> ("arg" here), that is we can't do a check for ')' to see if we have >> exhausted our option list. So we can't really use the same functions, but >> there is the possiblity that we can modify them to be used here too. > > That is the kind of "sharing" to reduce repetition I had in mind. > > I haven't checked the callers, but another way would be to update > the caller of for-each-ref's side to match the calling convention of > how pretty calls the parser, wouldn't it? After all, they parse the > same "%(token:key=val,key=val,...)" so...? Having said all that, let's move things forward by merging this version. Anybody (it could be you) interested in cleaning up by unifying these two very similar implementations of the same thing can do so on top. Thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v4 2/2] ref-filter: add new "describe" atom 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-23 16:19 ` Kousik Sanagavarapu 2023-07-24 17:21 ` Junio C Hamano 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu 2 siblings, 1 reply; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-23 16:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu Duplicate the logic of %(describe) and friends from pretty to ref-filter. In the future, this change helps in unifying both the formats as ref-filter will be able to do everything that pretty is doing and we can have a single interface. The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/git-for-each-ref.txt | 23 +++++ ref-filter.c | 125 ++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 138 +++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index d9588767a9..11b2bc3121 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -264,6 +264,29 @@ ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. +describe[:options]:: + A human-readable name, like linkgit:git-describe[1]; + empty string for undescribable commits. The `describe` string may + be followed by a colon and one or more comma-separated options. ++ +-- +tags=<bool-value>;; + Instead of only considering annotated tags, consider + lightweight tags as well; see the corresponding option in + linkgit:git-describe[1] for details. +abbrev=<number>;; + Use at least <number> hexadecimal digits; see the corresponding + option in linkgit:git-describe[1] for details. +match=<pattern>;; + Only consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding option + in linkgit:git-describe[1] for details. +exclude=<pattern>;; + Do not consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding option + in linkgit:git-describe[1] for details. +-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/ref-filter.c b/ref-filter.c index 8d5f85e0a7..df00f1628c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -5,6 +5,7 @@ #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" +#include "run-command.h" #include "refs.h" #include "wildmatch.h" #include "object-name.h" @@ -146,6 +147,7 @@ enum atom_type { ATOM_TAGGERDATE, ATOM_CREATOR, ATOM_CREATORDATE, + ATOM_DESCRIBE, ATOM_SUBJECT, ATOM_BODY, ATOM_TRAILERS, @@ -220,6 +222,7 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + const char **describe_args; struct refname_atom refname; char *head; } u; @@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato return 0; } +static int describe_atom_option_parser(struct strvec *args, const char **arg, + struct strbuf *err) +{ + const char *argval; + size_t arglen = 0; + int optval = 0; + + if (match_atom_bool_arg(*arg, "tags", arg, &optval)) { + if (!optval) + strvec_push(args, "--no-tags"); + else + strvec_push(args, "--tags"); + return 1; + } + + if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) { + char *endptr; + + if (!arglen) + return strbuf_addf_ret(err, -1, + _("argument expected for %s"), + "describe:abbrev"); + if (strtol(argval, &endptr, 10) < 0) + return strbuf_addf_ret(err, -1, + _("positive value expected %s=%s"), + "describe:abbrev", argval); + if (endptr - argval != arglen) + return strbuf_addf_ret(err, -1, + _("cannot fully parse %s=%s"), + "describe:abbrev", argval); + + strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:match"); + + strvec_pushf(args, "--match=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:exclude"); + + strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval); + return 1; + } + + return 0; +} + +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + struct strvec args = STRVEC_INIT; + + for (;;) { + int found = 0; + const char *bad_arg = arg; + + if (!arg || !*arg) + break; + + found = describe_atom_option_parser(&args, &arg, err); + if (found < 0) + return found; + if (!found) + return err_bad_arg(err, "describe", bad_arg); + } + atom->u.describe_args = strvec_detach(&args); + return 0; +} + static int raw_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) @@ -802,6 +886,7 @@ static struct { [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, @@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } } +static void grab_describe_values(struct atom_value *val, int deref, + struct object *obj) +{ + struct commit *commit = (struct commit *)obj; + int i; + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + enum atom_type type = atom->atom_type; + const char *name = atom->name; + struct atom_value *v = &val[i]; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + + if (type != ATOM_DESCRIBE) + continue; + + if (!!deref != (*name == '*')) + continue; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); + strvec_pushv(&cmd.args, atom->u.describe_args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); + v->s = xstrdup(""); + continue; + } + strbuf_rtrim(&out); + v->s = strbuf_detach(&out, NULL); + + strbuf_release(&err); + } +} + /* See grab_values */ static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data) { @@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_tag_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("tagger", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); @@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); grab_signature(val, deref, obj); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: /* grab_tree_values(val, deref, obj, buf, sz); */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 910bf1ea94..7116e008f4 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -597,6 +597,144 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' +test_expect_success 'setup for describe atom tests' ' + git init -b master describe-repo && + ( + cd describe-repo && + + test_commit --no-tag one && + git tag tagone && + + test_commit --no-tag two && + git tag -a -m "tag two" tagtwo + ) +' + +test_expect_success 'describe atom vs git describe' ' + ( + cd describe-repo && + + git for-each-ref --format="%(objectname)" \ + refs/tags/ >obj && + while read hash + do + if desc=$(git describe $hash) + then + : >expect-contains-good + else + : >expect-contains-bad + fi && + echo "$hash $desc" || return 1 + done <obj >expect && + test_path_exists expect-contains-good && + test_path_exists expect-contains-bad && + + git for-each-ref --format="%(objectname) %(describe)" \ + refs/tags/ >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + ) +' + +test_expect_success 'describe:tags vs describe --tags' ' + ( + cd describe-repo && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' + ( + cd describe-repo && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + test tagname = $(cat actual) + ) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' + ( + cd describe-repo && + git tag -a -m "tag foo" tag-foo && + git describe --match "*-foo" >expect && + git for-each-ref --format="%(describe:match="*-foo")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' + ( + cd describe-repo && + git tag -a -m "tag bar" tag-bar && + git describe --exclude "*-bar" >expect && + git for-each-ref --format="%(describe:exclude="*-bar")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'deref with describe atom' ' + ( + cd describe-repo && + cat >expect <<-\EOF && + + tagname + tagname + tagname + + tagtwo + EOF + git for-each-ref --format="%(*describe)" >actual && + test_cmp expect actual + ) +' + +test_expect_success 'err on bad describe atom arg' ' + ( + cd describe-repo && + + # The bad arg is the only arg passed to describe atom + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: baz + EOF + ! git for-each-ref --format="%(describe:baz)" \ + refs/heads/master 2>actual && + test_cmp expect actual && + + # The bad arg is in the middle of the option string + # passed to the describe atom + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: qux=1,abbrev=14 + EOF + ! git for-each-ref \ + --format="%(describe:tags,qux=1,abbrev=14)" \ + ref/heads/master 2>actual && + test_cmp expect actual + ) +' + cat >expected <<\EOF heads/main tags/main -- 2.41.0.396.g9ab76b0018 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v4 2/2] ref-filter: add new "describe" atom 2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-24 17:21 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-24 17:21 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > +test_expect_success 'err on bad describe atom arg' ' > + ( > + cd describe-repo && > + > + # The bad arg is the only arg passed to describe atom > + cat >expect <<-\EOF && > + fatal: unrecognized %(describe) argument: baz > + EOF > + ! git for-each-ref --format="%(describe:baz)" \ > + refs/heads/master 2>actual && > + test_cmp expect actual && Instead of "! git something", use of "test_must_fail git something" is recommended. The former would pass upon a crashing "git" happily, but the latter would complain if "git" segfaults. > + # The bad arg is in the middle of the option string > + # passed to the describe atom > + cat >expect <<-\EOF && > + fatal: unrecognized %(describe) argument: qux=1,abbrev=14 > + EOF > + ! git for-each-ref \ > + --format="%(describe:tags,qux=1,abbrev=14)" \ > + ref/heads/master 2>actual && Ditto. > + test_cmp expect actual > + ) > +' Other than that, both patches looked good to me. Thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v5 0/2] Add new "describe" atom 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-25 20:51 ` Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu ` (2 more replies) 2 siblings, 3 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu Hi, Thanks for the review on the previous version of this series. Here is a quick re-roll to address the minor changes that you left on the previous version (apart from the suggestions to PATCH 1/2). Please queue this instead. PATCH 1/2 - Left unchanged PATCH 2/2 - Used the test helper function `test_must_fail` instead of something like "! git foo" for making a command fail. Kousik Sanagavarapu (2): ref-filter: add multiple-option parsing functions ref-filter: add new "describe" atom Documentation/git-for-each-ref.txt | 23 +++ ref-filter.c | 230 +++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 139 +++++++++++++++++ 3 files changed, 392 insertions(+) Range-diff against v4: 1: 2914bd58ec = 1: 2914bd58ec ref-filter: add multiple-option parsing functions 2: 77a2a56520 ! 2: 8127f4399c ref-filter: add new "describe" atom @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: baz + EOF -+ ! git for-each-ref --format="%(describe:baz)" \ ++ test_must_fail git for-each-ref \ ++ --format="%(describe:baz)" \ + refs/heads/master 2>actual && + test_cmp expect actual && + @@ t/t6300-for-each-ref.sh: test_expect_success 'color.ui=always does not override + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: qux=1,abbrev=14 + EOF -+ ! git for-each-ref \ ++ test_must_fail git for-each-ref \ + --format="%(describe:tags,qux=1,abbrev=14)" \ + ref/heads/master 2>actual && + test_cmp expect actual -- 2.41.0.396.g77a2a56520 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v5 1/2] ref-filter: add multiple-option parsing functions 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu @ 2023-07-25 20:51 ` Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano 2 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu The functions match_placeholder_arg_value() match_placeholder_bool_arg() were added in pretty 4f732e0fd7 (pretty: allow %(trailers) options with explicit value, 2019-01-29) to parse multiple options in an argument to --pretty. For example, git log --pretty="%(trailers:key=Signed-Off-By,separator=%x2C )" will output all the trailers matching the key and seperates them by a comma followed by a space per commit. Add similar functions, match_atom_arg_value() match_atom_bool_arg() in ref-filter. There is no atom yet that can use these functions in ref-filter, but we are going to add a new %(describe) atom in a subsequent commit where we parse options like tags=<bool-value> or match=<pattern> given to it. Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- ref-filter.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/ref-filter.c b/ref-filter.c index 0f3df132b8..8d5f85e0a7 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1,6 +1,7 @@ #include "git-compat-util.h" #include "environment.h" #include "gettext.h" +#include "config.h" #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" @@ -255,6 +256,110 @@ static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg) return -1; } +/* + * Parse option of name "candidate" in the option string "to_parse" of + * the form + * + * "candidate1[=val1],candidate2[=val2],candidate3[=val3],..." + * + * The remaining part of "to_parse" is stored in "end" (if we are + * parsing the last candidate, then this is NULL) and the value of + * the candidate is stored in "valuestart" and its length in "valuelen", + * that is the portion after "=". Since it is possible for a "candidate" + * to not have a value, in such cases, "valuestart" is set to point to + * NULL and "valuelen" to 0. + * + * The function returns 1 on success. It returns 0 if we don't find + * "candidate" in "to_parse" or we find "candidate" but it is followed + * by more chars (for example, "candidatefoo"), that is, we don't find + * an exact match. + * + * This function only does the above for one "candidate" at a time. So + * it has to be called each time trying to parse a "candidate" in the + * option string "to_parse". + */ +static int match_atom_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) +{ + const char *atom; + + if (!skip_prefix(to_parse, candidate, &atom)) + return 0; /* definitely not "candidate" */ + + if (*atom == '=') { + /* we just saw "candidate=" */ + *valuestart = atom + 1; + atom = strchrnul(*valuestart, ','); + *valuelen = atom - *valuestart; + } else if (*atom != ',' && *atom != '\0') { + /* key begins with "candidate" but has more chars */ + return 0; + } else { + /* just "candidate" without "=val" */ + *valuestart = NULL; + *valuelen = 0; + } + + /* atom points at either the ',' or NUL after this key[=val] */ + if (*atom == ',') + atom++; + else if (*atom) + BUG("Why is *atom not NULL yet?"); + + *end = atom; + return 1; +} + +/* + * Parse boolean option of name "candidate" in the option list "to_parse" + * of the form + * + * "candidate1[=bool1],candidate2[=bool2],candidate3[=bool3],..." + * + * The remaining part of "to_parse" is stored in "end" (if we are parsing + * the last candidate, then this is NULL) and the value (if given) is + * parsed and stored in "val", so "val" always points to either 0 or 1. + * If the value is not given, then "val" is set to point to 1. + * + * The boolean value is parsed using "git_parse_maybe_bool()", so the + * accepted values are + * + * to set true - "1", "yes", "true" + * to set false - "0", "no", "false" + * + * This function returns 1 on success. It returns 0 when we don't find + * an exact match for "candidate" or when the boolean value given is + * not valid. + */ +static int match_atom_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *argval; + char *strval; + size_t arglen; + int v; + + if (!match_atom_arg_value(to_parse, candidate, end, &argval, &arglen)) + return 0; + + if (!argval) { + *val = 1; + return 1; + } + + strval = xstrndup(argval, arglen); + v = git_parse_maybe_bool(strval); + free(strval); + + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static int color_atom_parser(struct ref_format *format, struct used_atom *atom, const char *color_value, struct strbuf *err) { -- 2.41.0.396.g77a2a56520 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v5 2/2] ref-filter: add new "describe" atom 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu @ 2023-07-25 20:51 ` Kousik Sanagavarapu 2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano 2 siblings, 0 replies; 38+ messages in thread From: Kousik Sanagavarapu @ 2023-07-25 20:51 UTC (permalink / raw) To: git Cc: Junio C Hamano, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma, Kousik Sanagavarapu Duplicate the logic of %(describe) and friends from pretty to ref-filter. In the future, this change helps in unifying both the formats as ref-filter will be able to do everything that pretty is doing and we can have a single interface. The new atom "describe" and its friends are equivalent to the existing pretty formats with the same name. Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- Documentation/git-for-each-ref.txt | 23 +++++ ref-filter.c | 125 ++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 139 +++++++++++++++++++++++++++++ 3 files changed, 287 insertions(+) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index d9588767a9..11b2bc3121 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -264,6 +264,29 @@ ahead-behind:<committish>:: commits ahead and behind, respectively, when comparing the output ref to the `<committish>` specified in the format. +describe[:options]:: + A human-readable name, like linkgit:git-describe[1]; + empty string for undescribable commits. The `describe` string may + be followed by a colon and one or more comma-separated options. ++ +-- +tags=<bool-value>;; + Instead of only considering annotated tags, consider + lightweight tags as well; see the corresponding option in + linkgit:git-describe[1] for details. +abbrev=<number>;; + Use at least <number> hexadecimal digits; see the corresponding + option in linkgit:git-describe[1] for details. +match=<pattern>;; + Only consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding option + in linkgit:git-describe[1] for details. +exclude=<pattern>;; + Do not consider tags matching the given `glob(7)` pattern, + excluding the "refs/tags/" prefix; see the corresponding option + in linkgit:git-describe[1] for details. +-- + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/ref-filter.c b/ref-filter.c index 8d5f85e0a7..df00f1628c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -5,6 +5,7 @@ #include "gpg-interface.h" #include "hex.h" #include "parse-options.h" +#include "run-command.h" #include "refs.h" #include "wildmatch.h" #include "object-name.h" @@ -146,6 +147,7 @@ enum atom_type { ATOM_TAGGERDATE, ATOM_CREATOR, ATOM_CREATORDATE, + ATOM_DESCRIBE, ATOM_SUBJECT, ATOM_BODY, ATOM_TRAILERS, @@ -220,6 +222,7 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + const char **describe_args; struct refname_atom refname; char *head; } u; @@ -600,6 +603,87 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato return 0; } +static int describe_atom_option_parser(struct strvec *args, const char **arg, + struct strbuf *err) +{ + const char *argval; + size_t arglen = 0; + int optval = 0; + + if (match_atom_bool_arg(*arg, "tags", arg, &optval)) { + if (!optval) + strvec_push(args, "--no-tags"); + else + strvec_push(args, "--tags"); + return 1; + } + + if (match_atom_arg_value(*arg, "abbrev", arg, &argval, &arglen)) { + char *endptr; + + if (!arglen) + return strbuf_addf_ret(err, -1, + _("argument expected for %s"), + "describe:abbrev"); + if (strtol(argval, &endptr, 10) < 0) + return strbuf_addf_ret(err, -1, + _("positive value expected %s=%s"), + "describe:abbrev", argval); + if (endptr - argval != arglen) + return strbuf_addf_ret(err, -1, + _("cannot fully parse %s=%s"), + "describe:abbrev", argval); + + strvec_pushf(args, "--abbrev=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "match", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:match"); + + strvec_pushf(args, "--match=%.*s", (int)arglen, argval); + return 1; + } + + if (match_atom_arg_value(*arg, "exclude", arg, &argval, &arglen)) { + if (!arglen) + return strbuf_addf_ret(err, -1, + _("value expected %s="), + "describe:exclude"); + + strvec_pushf(args, "--exclude=%.*s", (int)arglen, argval); + return 1; + } + + return 0; +} + +static int describe_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + struct strvec args = STRVEC_INIT; + + for (;;) { + int found = 0; + const char *bad_arg = arg; + + if (!arg || !*arg) + break; + + found = describe_atom_option_parser(&args, &arg, err); + if (found < 0) + return found; + if (!found) + return err_bad_arg(err, "describe", bad_arg); + } + atom->u.describe_args = strvec_detach(&args); + return 0; +} + static int raw_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) @@ -802,6 +886,7 @@ static struct { [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME }, [ATOM_CREATOR] = { "creator", SOURCE_OBJ }, [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME }, + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser }, [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser }, [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser }, [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser }, @@ -1708,6 +1793,44 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } } +static void grab_describe_values(struct atom_value *val, int deref, + struct object *obj) +{ + struct commit *commit = (struct commit *)obj; + int i; + + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + enum atom_type type = atom->atom_type; + const char *name = atom->name; + struct atom_value *v = &val[i]; + + struct child_process cmd = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + + if (type != ATOM_DESCRIBE) + continue; + + if (!!deref != (*name == '*')) + continue; + + cmd.git_cmd = 1; + strvec_push(&cmd.args, "describe"); + strvec_pushv(&cmd.args, atom->u.describe_args); + strvec_push(&cmd.args, oid_to_hex(&commit->object.oid)); + if (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) { + error(_("failed to run 'describe'")); + v->s = xstrdup(""); + continue; + } + strbuf_rtrim(&out); + v->s = strbuf_detach(&out, NULL); + + strbuf_release(&err); + } +} + /* See grab_values */ static void grab_sub_body_contents(struct atom_value *val, int deref, struct expand_data *data) { @@ -1817,6 +1940,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_tag_values(val, deref, obj); grab_sub_body_contents(val, deref, data); grab_person("tagger", val, deref, buf); + grab_describe_values(val, deref, obj); break; case OBJ_COMMIT: grab_commit_values(val, deref, obj); @@ -1824,6 +1948,7 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, s grab_person("author", val, deref, buf); grab_person("committer", val, deref, buf); grab_signature(val, deref, obj); + grab_describe_values(val, deref, obj); break; case OBJ_TREE: /* grab_tree_values(val, deref, obj, buf, sz); */ diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 910bf1ea94..16082dccb7 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -597,6 +597,145 @@ test_expect_success 'color.ui=always does not override tty check' ' test_cmp expected.bare actual ' +test_expect_success 'setup for describe atom tests' ' + git init -b master describe-repo && + ( + cd describe-repo && + + test_commit --no-tag one && + git tag tagone && + + test_commit --no-tag two && + git tag -a -m "tag two" tagtwo + ) +' + +test_expect_success 'describe atom vs git describe' ' + ( + cd describe-repo && + + git for-each-ref --format="%(objectname)" \ + refs/tags/ >obj && + while read hash + do + if desc=$(git describe $hash) + then + : >expect-contains-good + else + : >expect-contains-bad + fi && + echo "$hash $desc" || return 1 + done <obj >expect && + test_path_exists expect-contains-good && + test_path_exists expect-contains-bad && + + git for-each-ref --format="%(objectname) %(describe)" \ + refs/tags/ >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + ) +' + +test_expect_success 'describe:tags vs describe --tags' ' + ( + cd describe-repo && + git describe --tags >expect && + git for-each-ref --format="%(describe:tags)" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:abbrev=... vs describe --abbrev=...' ' + ( + cd describe-repo && + + # Case 1: We have commits between HEAD and the most + # recent tag reachable from it + test_commit --no-tag file && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + + # Make sure the hash used is atleast 14 digits long + sed -e "s/^.*-g\([0-9a-f]*\)$/\1/" <actual >hexpart && + test 15 -le $(wc -c <hexpart) && + + # Case 2: We have a tag at HEAD, describe directly gives + # the name of the tag + git tag -a -m tagged tagname && + git describe --abbrev=14 >expect && + git for-each-ref --format="%(describe:abbrev=14)" \ + refs/heads/master >actual && + test_cmp expect actual && + test tagname = $(cat actual) + ) +' + +test_expect_success 'describe:match=... vs describe --match ...' ' + ( + cd describe-repo && + git tag -a -m "tag foo" tag-foo && + git describe --match "*-foo" >expect && + git for-each-ref --format="%(describe:match="*-foo")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe:exclude:... vs describe --exclude ...' ' + ( + cd describe-repo && + git tag -a -m "tag bar" tag-bar && + git describe --exclude "*-bar" >expect && + git for-each-ref --format="%(describe:exclude="*-bar")" \ + refs/heads/master >actual && + test_cmp expect actual + ) +' + +test_expect_success 'deref with describe atom' ' + ( + cd describe-repo && + cat >expect <<-\EOF && + + tagname + tagname + tagname + + tagtwo + EOF + git for-each-ref --format="%(*describe)" >actual && + test_cmp expect actual + ) +' + +test_expect_success 'err on bad describe atom arg' ' + ( + cd describe-repo && + + # The bad arg is the only arg passed to describe atom + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: baz + EOF + test_must_fail git for-each-ref \ + --format="%(describe:baz)" \ + refs/heads/master 2>actual && + test_cmp expect actual && + + # The bad arg is in the middle of the option string + # passed to the describe atom + cat >expect <<-\EOF && + fatal: unrecognized %(describe) argument: qux=1,abbrev=14 + EOF + test_must_fail git for-each-ref \ + --format="%(describe:tags,qux=1,abbrev=14)" \ + ref/heads/master 2>actual && + test_cmp expect actual + ) +' + cat >expected <<\EOF heads/main tags/main -- 2.41.0.396.g77a2a56520 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v5 0/2] Add new "describe" atom 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu @ 2023-07-25 21:46 ` Junio C Hamano 2 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2023-07-25 21:46 UTC (permalink / raw) To: Kousik Sanagavarapu Cc: git, Glen Choo, Josh Steadmon, Siddharth Singh, Christian Couder, Hariom Verma Kousik Sanagavarapu <five231003@gmail.com> writes: > Thanks for the review on the previous version of this series. Here is a > quick re-roll to address the minor changes that you left on the previous > version (apart from the suggestions to PATCH 1/2). > > Please queue this instead. "! -> test_must_fail" has already been corrected locally yesterday before I pushed the integration results out, so I'd skip this round. ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2023-07-25 21:47 UTC | newest] Thread overview: 38+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu 2023-07-06 16:58 ` Junio C Hamano 2023-07-09 6:16 ` Kousik Sanagavarapu 2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu 2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-14 20:57 ` Junio C Hamano 2023-07-15 18:24 ` Kousik Sanagavarapu 2023-07-15 18:56 ` Junio C Hamano 2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu 2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-19 23:23 ` Junio C Hamano 2023-07-20 5:21 ` Junio C Hamano 2023-07-20 16:52 ` Kousik Sanagavarapu 2023-07-20 17:59 ` Junio C Hamano 2023-07-20 17:42 ` Glen Choo 2023-07-20 20:30 ` Junio C Hamano 2023-07-21 18:26 ` Glen Choo 2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-19 22:56 ` Junio C Hamano 2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano 2023-07-20 23:10 ` Junio C Hamano 2023-07-21 4:17 ` Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu 2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-24 17:29 ` Junio C Hamano 2023-07-24 18:12 ` Kousik Sanagavarapu 2023-07-24 20:39 ` Junio C Hamano 2023-07-25 19:27 ` Junio C Hamano 2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-24 17:21 ` Junio C Hamano 2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu 2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu 2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).