| Message ID | 20230714194249.66862-3-five231003@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new "describe" atom | expand |
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.
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
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.
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
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(+)