On 10/24/21 12:57 AM, Junio C Hamano wrote: > Eli Schwartz writes: > >> The %(describe) placeholder by default, like `git describe`, only >> supports annotated tags. However, some people do use lightweight tags >> for releases, and would like to describe those anyway. The command line >> tool has an option to support this. >> >> Teach the placeholder to support this as well. >> >> Signed-off-by: Eli Schwartz >> --- >> Documentation/pretty-formats.txt | 11 ++++++----- >> pretty.c | 23 +++++++++++++++++++---- >> t/t4205-log-pretty-formats.sh | 8 ++++++++ >> 3 files changed, 33 insertions(+), 9 deletions(-) >> >> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt >> index ef6bd420ae..14107ac191 100644 >> --- a/Documentation/pretty-formats.txt >> +++ b/Documentation/pretty-formats.txt >> @@ -220,6 +220,7 @@ The placeholders are: >> inconsistent when tags are added or removed at >> the same time. >> + >> +** 'tags[=]': Also consider lightweight tags. >> ** 'match=': Only consider tags matching the given >> `glob(7)` pattern, excluding the "refs/tags/" prefix. >> ** 'exclude=': Do not consider tags matching the given > > What is the guiding principle used in this patch to decide where the > new entry should go? > > The existing 'match' and 'exclude' are the opposites to each other, > and it makes sense to keep them together, and between them, 'match' > is the positive variant while 'exclude' is the negative one, so the > current order makes sense. I wonder why the new "also consider" > should come before them, instead of after. > > I am not saying you should change the order, and I would be most > unhappy if you did so without explanation in an updated patch. > Rather, I would like to hear the reasoning behind the decision, > preferrably in the proposed log message. The guiding principle I used was to replicate the order in which the same options are listed in git-describe(1). Err, maybe that means I should not have used the word "also" in the doc string... >> @@ -273,11 +274,6 @@ endif::git-rev-list[] >> If any option is provided multiple times the >> last occurrence wins. >> + >> -The boolean options accept an optional value `[=]`. The values >> -`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean" >> -sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean >> -option is given with no value, it's enabled. >> -+ >> ** 'key=': only show trailers with specified key. Matching is done >> case-insensitively and trailing colon is optional. If option is >> given multiple times trailer lines matching any of the keys are >> @@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by >> decoration format if `--decorate` was not already provided on the command >> line. >> >> +The boolean options accept an optional value `[=]`. The values >> +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean" >> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean >> +option is given with no value, it's enabled. >> + > > This paragraph used to be inside the description of %(trailers:...), > but that was only because %(trailers) was the only one that took a > boolean value for its options, and not because it was the only one > that had special treatment for its boolean options. Because the > existing rule for an option that takes a boolean value equally > applies to the %(describe:...), and more importantly, because we > expect that any other pretty-format placeholder that would acquire > an option with boolean value would follow the same rule, it makes > sense to move it here, together with other rules like %+ and %- that > apply to any placeholders. > > Makes sense. I very much appreciate this extra attention to the > detail. :) >> diff --git a/pretty.c b/pretty.c >> index 9db2c65538..3a41bedf1a 100644 >> --- a/pretty.c >> +++ b/pretty.c >> @@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts, >> >> static size_t parse_describe_args(const char *start, struct strvec *args) >> { >> + const char *options[] = { "tags" }; >> const char *option_arguments[] = { "match", "exclude" }; >> const char *arg = start; >> >> for (;;) { >> const char *matched = NULL; >> - const char *argval; >> + const char *argval = NULL; >> size_t arglen = 0; >> + int optval = 0; >> int i; >> >> for (i = 0; i < ARRAY_SIZE(option_arguments); i++) { >> if (match_placeholder_arg_value(arg, option_arguments[i], &arg, >> &argval, &arglen)) { >> matched = option_arguments[i]; >> + if (!arglen) >> + return 0; >> break; >> } >> } >> + if (!matched) >> + for (i = 0; i < ARRAY_SIZE(options); i++) { >> + if (match_placeholder_bool_arg(arg, options[i], &arg, >> + &optval)) { >> + matched = options[i]; >> + break; >> + } >> + } >> if (!matched) >> break; >> > > I find this new structure of the code somewhat dubious. Shouldn't > we be rather starting with an array of struct that describes the > token to match and how the token should be handled? Something like > > struct { > const char *name; > enum { OPT_STRING, OPT_BOOL } type; > } option[] = { > { "exclude", OPT_STRING }, > { "match", OPT_STRING }, > { "tags", OPT_BOOL }, > }; > > for (;;) { > int i; > int found = 0; > ... > for (i = 0; !found && i < ARRAY_SIZE(option); i++) { > switch (option.type) { > case OPT_STRING: > if (match_placeholder_arg_value(...)) { > strvec_pushf(args, "--%s=%.*s", ...); > found = 1; > } > break; > case OPT_BOOL: > if (match_placeholder_bool_arg(...)) { > found = 1; > if (optval) > strvec_pushf(args, "--%s", option.name); > else > strvec_pushf(args, "--no-%s", option.name); > } > break; > } > } > } > > And instead of the option -> option_arguments rename, the 1/3 of the > series can be to introduce the above structure, without introducing > OPT_BOOL and "tags" element to the option[] array. Maybe! I'll confess that I'm a bit of a monkey-see-monkey-do coder when it comes to C (I keep meaning to learn it properly, but as things stand I'm a lot more comfortable in e.g. python). So there is a good chance I could be a lot more optimized in my approach... your suggestion resembles the kind of thing I might do in a language I know better. I'll look into this, it makes sense. >> - if (!arglen) >> - return 0; >> - strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval); >> + >> + if (argval) { >> + strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval); >> + } else if (optval) { >> + strvec_pushf(args, "--%s", matched); >> + } >> } >> return arg - start; >> } >> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh >> index 5865daa8f8..d4acf8882f 100755 >> --- a/t/t4205-log-pretty-formats.sh >> +++ b/t/t4205-log-pretty-formats.sh >> @@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' ' >> test_cmp expect actual >> ' >> >> +test_expect_success '%(describe:tags) vs git describe --tags' ' >> + test_when_finished "git tag -d tagname" && >> + git tag tagname && >> + git describe --tags >expect && >> + git log -1 --format="%(describe:tags)" >actual && >> + test_cmp expect actual >> +' > > Nice. > > I like how the end-user visible part of this addition is designed to > look very much. With a cleaned up implementation it would be great. > > Thanks. > -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User