| Message ID | 20230719162424.70781-1-five231003@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add new "describe" atom | expand |
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
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 &&
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
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