{"thread":{"id":"56770","subject":"[PATCH 1/3] pretty.c: rename describe options variable to more descriptive name","startedAt":"2021-10-24T01:51:14Z","lastAt":"2021-11-07T12:40:19Z","messageCount":46,"participants":["Eli Schwartz","Junio C Hamano","Eric Sunshine","Đoàn Trần Công Danh","Carlo Arenas","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"439471","messageId":"20211024014256.3569322-2-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211024014256.3569322-1-eschwartz@archlinux.org","subject":"[PATCH 1/3] pretty.c: rename describe options variable to more descriptive name","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T01:42:54Z","receivedAt":"2021-10-24T01:51:14Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"It contains option arguments, not options. We would like to add option\nsupport here too.\n\nSigned-off-by: Eli Schwartz \n---\n pretty.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..9db2c65538 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n-\tconst char *options[] = { \"match\", \"exclude\" };\n+\tconst char *option_arguments[] = { \"match\", \"exclude\" };\n \tconst char *arg = start;\n \n \tfor (;;) {\n@@ -1225,10 +1225,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tsize_t arglen = 0;\n \t\tint i;\n \n-\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n-\t\t\tif (match_placeholder_arg_value(arg, options[i], &arg,\n+\t\tfor (i = 0; i < ARRAY_SIZE(option_arguments); i++) {\n+\t\t\tif (match_placeholder_arg_value(arg, option_arguments[i], &arg,\n \t\t\t\t\t\t\t&argval, &arglen)) {\n-\t\t\t\tmatched = options[i];\n+\t\t\t\tmatched = option_arguments[i];\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-- \n2.33.1\n\n"},{"id":"439472","messageId":"20211024014256.3569322-1-eschwartz@archlinux.org","threadId":"56770","inReplyTo":null,"subject":"[PATCH 0/3] Add some more options to the pretty-formats","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T01:42:53Z","receivedAt":"2021-10-24T01:51:14Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"While discussing adding git archive support to a software versioning\ntool, the issue came up that apparently many people (maybe in the python\ncommunity specifically?) use lightweight tags for releases.\n\nWhile it would be nice if the current describe functionality for\nexport-subst was sufficient to cover the average reasonable use, this is\napparently not the case; at least --tags support will most likely need\nto be added. The alternative is documenting a workflow change and having\nthe versioning tool raise some kind of error if you use the wrong kind\nof tag, which is not an exciting requirement for the project maintainer.\n\nIn my initial proposal of the %(describe) feature I gave an example\nusing --tags, but it never ended up in the initial implementation:\n\nhttps://public-inbox.org/git/7418f1d8-78c2-61a7-4f03-62360b986a41@archlinux.org/\n\nSo I figured I'd take a stab at it myself. While I was at it, I looked\nat the options available to git describe and came up with a use case for\nadding --abbrev support too.\n\nEli Schwartz (3):\n pretty.c: rename describe options variable to more descriptive name\n pretty: add tag option to %(describe)\n pretty: add abbrev option to %(describe)\n\n Documentation/pretty-formats.txt | 15 ++++++++++-----\n pretty.c | 31 +++++++++++++++++++++++--------\n t/t4205-log-pretty-formats.sh | 16 ++++++++++++++++\n 3 files changed, 49 insertions(+), 13 deletions(-)\n\n-- \n2.33.1\n\n"},{"id":"439473","messageId":"20211024014256.3569322-4-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211024014256.3569322-1-eschwartz@archlinux.org","subject":"[PATCH 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T01:42:56Z","receivedAt":"2021-10-24T01:51:14Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, uses a\nseven-character abbreviated commit hash. This may not be sufficient to\nfully describe all git repos, resulting in a placeholder replacement\nchanging its length because the repository grew in size. This could\ncause the output of git-archive to change.\n\nAdd the --abbrev option to `git describe` to the placeholder interface\nin order to provide tools to the user for fine-tuning project defaults\nand ensure reproducible archives.\n\nOne alternative would be to just always specify --abbrev=40 but this may\nbe a bit too biased...\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 2 +-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 14107ac191..317c1382b5 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -221,6 +221,10 @@ The placeholders are:\n \t\t\t the same time.\n +\n ** 'tags[=]': Also consider lightweight tags.\n+** 'abbrev=': Instead of using the default number of hexadecimal digits\n+ (which will vary according to the number of objects in the repository with a\n+ default of 7) of the abbreviated object name, use digits, or as many digits\n+ as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\ndiff --git a/pretty.c b/pretty.c\nindex 3a41bedf1a..a092457274 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1217,7 +1217,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n \tconst char *options[] = { \"tags\" };\n-\tconst char *option_arguments[] = { \"match\", \"exclude\" };\n+\tconst char *option_arguments[] = { \"match\", \"exclude\", \"abbrev\" };\n \tconst char *arg = start;\n \n \tfor (;;) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex d4acf8882f..35eef4c865 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag -a -m tagged tagname &&\n+\tgit describe --abbrev=15 >expect &&\n+\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"439474","messageId":"20211024014256.3569322-3-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211024014256.3569322-1-eschwartz@archlinux.org","subject":"[PATCH 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T01:42:55Z","receivedAt":"2021-10-24T01:51:14Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, only\nsupports annotated tags. However, some people do use lightweight tags\nfor releases, and would like to describe those anyway. The command line\ntool has an option to support this.\n\nTeach the placeholder to support this as well.\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 11 ++++++-----\n pretty.c | 23 +++++++++++++++++++----\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex ef6bd420ae..14107ac191 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -220,6 +220,7 @@ The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n+** 'tags[=]': Also consider lightweight tags.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n@@ -273,11 +274,6 @@ endif::git-rev-list[]\n \t\t\t If any option is provided multiple times the\n \t\t\t last occurrence wins.\n +\n-The boolean options accept an optional value `[=]`. The values\n-`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n-sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n-option is given with no value, it's enabled.\n-+\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n@@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n decoration format if `--decorate` was not already provided on the command\n line.\n \n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n+\n If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\ndiff --git a/pretty.c b/pretty.c\nindex 9db2c65538..3a41bedf1a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n+\tconst char *options[] = { \"tags\" };\n \tconst char *option_arguments[] = { \"match\", \"exclude\" };\n \tconst char *arg = start;\n \n \tfor (;;) {\n \t\tconst char *matched = NULL;\n-\t\tconst char *argval;\n+\t\tconst char *argval = NULL;\n \t\tsize_t arglen = 0;\n+\t\tint optval = 0;\n \t\tint i;\n \n \t\tfor (i = 0; i < ARRAY_SIZE(option_arguments); i++) {\n \t\t\tif (match_placeholder_arg_value(arg, option_arguments[i], &arg,\n \t\t\t\t\t\t\t&argval, &arglen)) {\n \t\t\t\tmatched = option_arguments[i];\n+\t\t\t\tif (!arglen)\n+\t\t\t\t\treturn 0;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n+\t\tif (!matched)\n+\t\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n+\t\t\t\tif (match_placeholder_bool_arg(arg, options[i], &arg,\n+\t\t\t\t\t\t\t\t&optval)) {\n+\t\t\t\t\tmatched = options[i];\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n \t\tif (!matched)\n \t\t\tbreak;\n \n-\t\tif (!arglen)\n-\t\t\treturn 0;\n-\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n+\n+\t\tif (argval) {\n+\t\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n+\t\t} else if (optval) {\n+\t\t\tstrvec_pushf(args, \"--%s\", matched);\n+\t\t}\n \t}\n \treturn arg - start;\n }\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5865daa8f8..d4acf8882f 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:tags) vs git describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"439475","messageId":"xmqqv91n6o1u.fsf@gitster.g","threadId":"56770","inReplyTo":"20211024014256.3569322-2-eschwartz@archlinux.org","subject":"Re: [PATCH 1/3] pretty.c: rename describe options variable to more descriptive name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T04:31:57Z","receivedAt":"2021-10-24T04:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> It contains option arguments, not options. We would like to add option\n> support here too.\n>\n> Signed-off-by: Eli Schwartz \n> ---\n> pretty.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index 73b5ead509..9db2c65538 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n> \n> static size_t parse_describe_args(const char *start, struct strvec *args)\n> {\n> -\tconst char *options[] = { \"match\", \"exclude\" };\n> +\tconst char *option_arguments[] = { \"match\", \"exclude\" };\n\nThis renaming is more or less \"Meh\" without the other change in the\nseries that may (or may not) be helped with this step, but because I\nhaven't seen these later steps yet, I may sound too dismissive of\nthe change in this step.\n\nAnyway, at least call that option_args[] to match the way the\nfunction calls itself, not option_arguments[] that is a mouthful for\na mere implementation detail of local variable for a file-private\nhelper function.\n\nThanks.\n\n> \tconst char *arg = start;\n> \n> \tfor (;;) {\n> @@ -1225,10 +1225,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n> \t\tsize_t arglen = 0;\n> \t\tint i;\n> \n> -\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n> -\t\t\tif (match_placeholder_arg_value(arg, options[i], &arg,\n> +\t\tfor (i = 0; i < ARRAY_SIZE(option_arguments); i++) {\n> +\t\t\tif (match_placeholder_arg_value(arg, option_arguments[i], &arg,\n> \t\t\t\t\t\t\t&argval, &arglen)) {\n> -\t\t\t\tmatched = options[i];\n> +\t\t\t\tmatched = option_arguments[i];\n> \t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n"},{"id":"439476","messageId":"xmqq5ytn6mw2.fsf@gitster.g","threadId":"56770","inReplyTo":"20211024014256.3569322-3-eschwartz@archlinux.org","subject":"Re: [PATCH 2/3] pretty: add tag option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T04:57:01Z","receivedAt":"2021-10-24T04:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> The %(describe) placeholder by default, like `git describe`, only\n> supports annotated tags. However, some people do use lightweight tags\n> for releases, and would like to describe those anyway. The command line\n> tool has an option to support this.\n>\n> Teach the placeholder to support this as well.\n>\n> Signed-off-by: Eli Schwartz \n> ---\n> Documentation/pretty-formats.txt | 11 ++++++-----\n> pretty.c | 23 +++++++++++++++++++----\n> t/t4205-log-pretty-formats.sh | 8 ++++++++\n> 3 files changed, 33 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index ef6bd420ae..14107ac191 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -220,6 +220,7 @@ The placeholders are:\n> \t\t\t inconsistent when tags are added or removed at\n> \t\t\t the same time.\n> +\n> +** 'tags[=]': Also consider lightweight tags.\n> ** 'match=': Only consider tags matching the given\n> `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n> ** 'exclude=': Do not consider tags matching the given\n\nWhat is the guiding principle used in this patch to decide where the\nnew entry should go? \n\nThe existing 'match' and 'exclude' are the opposites to each other,\nand it makes sense to keep them together, and between them, 'match'\nis the positive variant while 'exclude' is the negative one, so the\ncurrent order makes sense. I wonder why the new \"also consider\"\nshould come before them, instead of after.\n\nI am not saying you should change the order, and I would be most\nunhappy if you did so without explanation in an updated patch.\nRather, I would like to hear the reasoning behind the decision,\npreferrably in the proposed log message.\n\n> @@ -273,11 +274,6 @@ endif::git-rev-list[]\n> \t\t\t If any option is provided multiple times the\n> \t\t\t last occurrence wins.\n> +\n> -The boolean options accept an optional value `[=]`. The values\n> -`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n> -sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n> -option is given with no value, it's enabled.\n> -+\n> ** 'key=': only show trailers with specified key. Matching is done\n> case-insensitively and trailing colon is optional. If option is\n> given multiple times trailer lines matching any of the keys are\n> @@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n> decoration format if `--decorate` was not already provided on the command\n> line.\n> \n> +The boolean options accept an optional value `[=]`. The values\n> +`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n> +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n> +option is given with no value, it's enabled.\n> +\n\nThis paragraph used to be inside the description of %(trailers:...),\nbut that was only because %(trailers) was the only one that took a\nboolean value for its options, and not because it was the only one\nthat had special treatment for its boolean options. Because the\nexisting rule for an option that takes a boolean value equally\napplies to the %(describe:...), and more importantly, because we\nexpect that any other pretty-format placeholder that would acquire\nan option with boolean value would follow the same rule, it makes\nsense to move it here, together with other rules like %+ and %- that\napply to any placeholders.\n\nMakes sense. I very much appreciate this extra attention to the\ndetail.\n\n\n> diff --git a/pretty.c b/pretty.c\n> index 9db2c65538..3a41bedf1a 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n> \n> static size_t parse_describe_args(const char *start, struct strvec *args)\n> {\n> +\tconst char *options[] = { \"tags\" };\n> \tconst char *option_arguments[] = { \"match\", \"exclude\" };\n> \tconst char *arg = start;\n> \n> \tfor (;;) {\n> \t\tconst char *matched = NULL;\n> -\t\tconst char *argval;\n> +\t\tconst char *argval = NULL;\n> \t\tsize_t arglen = 0;\n> +\t\tint optval = 0;\n> \t\tint i;\n> \n> \t\tfor (i = 0; i < ARRAY_SIZE(option_arguments); i++) {\n> \t\t\tif (match_placeholder_arg_value(arg, option_arguments[i], &arg,\n> \t\t\t\t\t\t\t&argval, &arglen)) {\n> \t\t\t\tmatched = option_arguments[i];\n> +\t\t\t\tif (!arglen)\n> +\t\t\t\t\treturn 0;\n> \t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n> +\t\tif (!matched)\n> +\t\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n> +\t\t\t\tif (match_placeholder_bool_arg(arg, options[i], &arg,\n> +\t\t\t\t\t\t\t\t&optval)) {\n> +\t\t\t\t\tmatched = options[i];\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t}\n> \t\tif (!matched)\n> \t\t\tbreak;\n> \n\nI find this new structure of the code somewhat dubious. Shouldn't\nwe be rather starting with an array of struct that describes the\ntoken to match and how the token should be handled? Something like\n\n struct {\n\tconst char *name;\n\tenum { OPT_STRING, OPT_BOOL } type;\n } option[] = {\n\t{ \"exclude\", OPT_STRING },\n { \"match\", OPT_STRING },\n\t{ \"tags\", OPT_BOOL },\n };\n\n for (;;) {\n\tint i;\n\tint found = 0;\n\t...\n for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n switch (option.type) {\n case OPT_STRING:\n if (match_placeholder_arg_value(...)) {\n strvec_pushf(args, \"--%s=%.*s\", ...);\n\t\t found = 1;\n\t\t}\n break;\n case OPT_BOOL:\n if (match_placeholder_bool_arg(...)) {\n\t\t found = 1;\n\t\t if (optval)\n\t\t\tstrvec_pushf(args, \"--%s\", option.name);\n\t\t else\n\t\t\tstrvec_pushf(args, \"--no-%s\", option.name);\n\t\t}\n\t\tbreak;\n\t }\n\t}\n }\n\nAnd instead of the option -> option_arguments rename, the 1/3 of the\nseries can be to introduce the above structure, without introducing\nOPT_BOOL and \"tags\" element to the option[] array.\n\n> -\t\tif (!arglen)\n> -\t\t\treturn 0;\n> -\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n> +\n> +\t\tif (argval) {\n> +\t\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n> +\t\t} else if (optval) {\n> +\t\t\tstrvec_pushf(args, \"--%s\", matched);\n> +\t\t}\n> \t}\n> \treturn arg - start;\n> }\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index 5865daa8f8..d4acf8882f 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '%(describe:tags) vs git describe --tags' '\n> +\ttest_when_finished \"git tag -d tagname\" &&\n> +\tgit tag tagname &&\n> +\tgit describe --tags >expect &&\n> +\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nNice.\n\nI like how the end-user visible part of this addition is designed to\nlook very much. With a cleaned up implementation it would be great.\n\nThanks.\n\n"},{"id":"439477","messageId":"xmqqv91n57g4.fsf@gitster.g","threadId":"56770","inReplyTo":"20211024014256.3569322-4-eschwartz@archlinux.org","subject":"Re: [PATCH 3/3] pretty: add abbrev option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T05:15:55Z","receivedAt":"2021-10-24T05:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> The %(describe) placeholder by default, like `git describe`, uses a\n> seven-character abbreviated commit hash. This may not be sufficient to\n\n\"hash\" -> \"object name\".\n\n> fully describe all git repos, resulting in a placeholder replacement\n\n\"all git repos\" -> \"all commits in a given repository\" (there may be\nother valid way to clarify, but the point is that 'describe' does\nnot describe 'git repos' in the sense that my repository gets\ndescription X while your repository gets description Y).\n\n> changing its length because the repository grew in size. This could\n> cause the output of git-archive to change.\n>\n> Add the --abbrev option to `git describe` to the placeholder interface\n> in order to provide tools to the user for fine-tuning project defaults\n> and ensure reproducible archives.\n\nNote that it is sad that --abbrev= does not necessarily ensure\nreproducibility. To be more precise, I do not think it sacrifices\nuniqueness to make the output reproducible. You can get more than N\nhex-digits in the output if N is too small to ensure uniquness.\n\nSo it indeed is that this line of thought ...\n\n> One alternative would be to just always specify --abbrev=40 but this may\n> be a bit too biased...\n\n... to use --abbrev=999 (because 40 is not the length of a full\nobject name in the SHA-2 world) is the only reasonable way, if what\nyou care about is the reproducibility.\n\n Side note. I think \"git describe --no-abbrev\" is buggy in that\n it does not give a full object name; I didn't check the code,\n but it appears to be behaving the same way as \"git describe\n --abbrev=0\" (show no hexdigits). Fixing this bug may possibly\n be a low-hanging fruit.\n\nBut even if the feature cannot be used to guarantee a full\nreproducibility, it is a good thing that we can now add this feature\nwith minimum effort thanks to the previous two steps.\n\nThe refactoring I suggested in my review for the previous step will\nshine, if we want to do a good job parsing the --abbrev= option,\nsince such a code organization would make it a fairly easy addition\nto introduce \"integer\" type that calls match_placeholder_arg_value()\nto read the option value (like \"string\" does) and validate that the\nvalue is indeed an integer.\n\nWould we want to support \"--contains\" as another boolean type? How\nabout \"--all\" and \"--long\"? All three sound plausible candidates.\n\nThanks.\n"},{"id":"439485","messageId":"bf4d8ed5-f168-6e9a-fb27-df735ff28eb6@archlinux.org","threadId":"56770","inReplyTo":"xmqqv91n6o1u.fsf@gitster.g","subject":"Re: [PATCH 1/3] pretty.c: rename describe options variable to more descriptive name","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T15:37:31Z","receivedAt":"2021-10-24T15:37:44Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/24/21 12:31 AM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> It contains option arguments, not options. We would like to add option\n>> support here too.\n>>\n>> Signed-off-by: Eli Schwartz \n>> ---\n>> pretty.c | 8 ++++----\n>> 1 file changed, 4 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/pretty.c b/pretty.c\n>> index 73b5ead509..9db2c65538 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n>> \n>> static size_t parse_describe_args(const char *start, struct strvec *args)\n>> {\n>> -\tconst char *options[] = { \"match\", \"exclude\" };\n>> +\tconst char *option_arguments[] = { \"match\", \"exclude\" };\n> \n> This renaming is more or less \"Meh\" without the other change in the\n> series that may (or may not) be helped with this step, but because I\n> haven't seen these later steps yet, I may sound too dismissive of\n> the change in this step.\n> \n> Anyway, at least call that option_args[] to match the way the\n> function calls itself, not option_arguments[] that is a mouthful for\n> a mere implementation detail of local variable for a file-private\n> helper function.\n\n\nRight, the reason this change was submitted in its own patch was\nessentially for the sole purpose of making the diff for the second patch\nintuitive to read.\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439486","messageId":"b08a9b79-c53b-c2f3-f976-01822b362add@archlinux.org","threadId":"56770","inReplyTo":"xmqq5ytn6mw2.fsf@gitster.g","subject":"Re: [PATCH 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T15:38:20Z","receivedAt":"2021-10-24T15:38:26Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/24/21 12:57 AM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> The %(describe) placeholder by default, like `git describe`, only\n>> supports annotated tags. However, some people do use lightweight tags\n>> for releases, and would like to describe those anyway. The command line\n>> tool has an option to support this.\n>>\n>> Teach the placeholder to support this as well.\n>>\n>> Signed-off-by: Eli Schwartz \n>> ---\n>> Documentation/pretty-formats.txt | 11 ++++++-----\n>> pretty.c | 23 +++++++++++++++++++----\n>> t/t4205-log-pretty-formats.sh | 8 ++++++++\n>> 3 files changed, 33 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> index ef6bd420ae..14107ac191 100644\n>> --- a/Documentation/pretty-formats.txt\n>> +++ b/Documentation/pretty-formats.txt\n>> @@ -220,6 +220,7 @@ The placeholders are:\n>> \t\t\t inconsistent when tags are added or removed at\n>> \t\t\t the same time.\n>> +\n>> +** 'tags[=]': Also consider lightweight tags.\n>> ** 'match=': Only consider tags matching the given\n>> `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n>> ** 'exclude=': Do not consider tags matching the given\n> \n> What is the guiding principle used in this patch to decide where the\n> new entry should go? \n> \n> The existing 'match' and 'exclude' are the opposites to each other,\n> and it makes sense to keep them together, and between them, 'match'\n> is the positive variant while 'exclude' is the negative one, so the\n> current order makes sense. I wonder why the new \"also consider\"\n> should come before them, instead of after.\n> \n> I am not saying you should change the order, and I would be most\n> unhappy if you did so without explanation in an updated patch.\n> Rather, I would like to hear the reasoning behind the decision,\n> preferrably in the proposed log message.\n\n\nThe guiding principle I used was to replicate the order in which the\nsame options are listed in git-describe(1).\n\nErr, maybe that means I should not have used the word \"also\" in the doc\nstring...\n\n\n>> @@ -273,11 +274,6 @@ endif::git-rev-list[]\n>> \t\t\t If any option is provided multiple times the\n>> \t\t\t last occurrence wins.\n>> +\n>> -The boolean options accept an optional value `[=]`. The values\n>> -`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n>> -sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n>> -option is given with no value, it's enabled.\n>> -+\n>> ** 'key=': only show trailers with specified key. Matching is done\n>> case-insensitively and trailing colon is optional. If option is\n>> given multiple times trailer lines matching any of the keys are\n>> @@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n>> decoration format if `--decorate` was not already provided on the command\n>> line.\n>> \n>> +The boolean options accept an optional value `[=]`. The values\n>> +`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n>> +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n>> +option is given with no value, it's enabled.\n>> +\n> \n> This paragraph used to be inside the description of %(trailers:...),\n> but that was only because %(trailers) was the only one that took a\n> boolean value for its options, and not because it was the only one\n> that had special treatment for its boolean options. Because the\n> existing rule for an option that takes a boolean value equally\n> applies to the %(describe:...), and more importantly, because we\n> expect that any other pretty-format placeholder that would acquire\n> an option with boolean value would follow the same rule, it makes\n> sense to move it here, together with other rules like %+ and %- that\n> apply to any placeholders.\n> \n> Makes sense. I very much appreciate this extra attention to the\n> detail.\n\n\n:)\n\n\n>> diff --git a/pretty.c b/pretty.c\n>> index 9db2c65538..3a41bedf1a 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n>> \n>> static size_t parse_describe_args(const char *start, struct strvec *args)\n>> {\n>> +\tconst char *options[] = { \"tags\" };\n>> \tconst char *option_arguments[] = { \"match\", \"exclude\" };\n>> \tconst char *arg = start;\n>> \n>> \tfor (;;) {\n>> \t\tconst char *matched = NULL;\n>> -\t\tconst char *argval;\n>> +\t\tconst char *argval = NULL;\n>> \t\tsize_t arglen = 0;\n>> +\t\tint optval = 0;\n>> \t\tint i;\n>> \n>> \t\tfor (i = 0; i < ARRAY_SIZE(option_arguments); i++) {\n>> \t\t\tif (match_placeholder_arg_value(arg, option_arguments[i], &arg,\n>> \t\t\t\t\t\t\t&argval, &arglen)) {\n>> \t\t\t\tmatched = option_arguments[i];\n>> +\t\t\t\tif (!arglen)\n>> +\t\t\t\t\treturn 0;\n>> \t\t\t\tbreak;\n>> \t\t\t}\n>> \t\t}\n>> +\t\tif (!matched)\n>> +\t\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n>> +\t\t\t\tif (match_placeholder_bool_arg(arg, options[i], &arg,\n>> +\t\t\t\t\t\t\t\t&optval)) {\n>> +\t\t\t\t\tmatched = options[i];\n>> +\t\t\t\t\tbreak;\n>> +\t\t\t\t}\n>> +\t\t\t}\n>> \t\tif (!matched)\n>> \t\t\tbreak;\n>> \n> \n> I find this new structure of the code somewhat dubious. Shouldn't\n> we be rather starting with an array of struct that describes the\n> token to match and how the token should be handled? Something like\n> \n> struct {\n> \tconst char *name;\n> \tenum { OPT_STRING, OPT_BOOL } type;\n> } option[] = {\n> \t{ \"exclude\", OPT_STRING },\n> { \"match\", OPT_STRING },\n> \t{ \"tags\", OPT_BOOL },\n> };\n> \n> for (;;) {\n> \tint i;\n> \tint found = 0;\n> \t...\n> for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n> switch (option.type) {\n> case OPT_STRING:\n> if (match_placeholder_arg_value(...)) {\n> strvec_pushf(args, \"--%s=%.*s\", ...);\n> \t\t found = 1;\n> \t\t}\n> break;\n> case OPT_BOOL:\n> if (match_placeholder_bool_arg(...)) {\n> \t\t found = 1;\n> \t\t if (optval)\n> \t\t\tstrvec_pushf(args, \"--%s\", option.name);\n> \t\t else\n> \t\t\tstrvec_pushf(args, \"--no-%s\", option.name);\n> \t\t}\n> \t\tbreak;\n> \t }\n> \t}\n> }\n> \n> And instead of the option -> option_arguments rename, the 1/3 of the\n> series can be to introduce the above structure, without introducing\n> OPT_BOOL and \"tags\" element to the option[] array.\n\n\nMaybe!\n\nI'll confess that I'm a bit of a monkey-see-monkey-do coder when it\ncomes to C (I keep meaning to learn it properly, but as things stand I'm\na lot more comfortable in e.g. python). So there is a good chance I\ncould be a lot more optimized in my approach... your suggestion\nresembles the kind of thing I might do in a language I know better.\n\nI'll look into this, it makes sense.\n\n\n>> -\t\tif (!arglen)\n>> -\t\t\treturn 0;\n>> -\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n>> +\n>> +\t\tif (argval) {\n>> +\t\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n>> +\t\t} else if (optval) {\n>> +\t\t\tstrvec_pushf(args, \"--%s\", matched);\n>> +\t\t}\n>> \t}\n>> \treturn arg - start;\n>> }\n>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n>> index 5865daa8f8..d4acf8882f 100755\n>> --- a/t/t4205-log-pretty-formats.sh\n>> +++ b/t/t4205-log-pretty-formats.sh\n>> @@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n>> \ttest_cmp expect actual\n>> '\n>> \n>> +test_expect_success '%(describe:tags) vs git describe --tags' '\n>> +\ttest_when_finished \"git tag -d tagname\" &&\n>> +\tgit tag tagname &&\n>> +\tgit describe --tags >expect &&\n>> +\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n> \n> Nice.\n> \n> I like how the end-user visible part of this addition is designed to\n> look very much. With a cleaned up implementation it would be great.\n> \n> Thanks.\n> \n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439487","messageId":"348299a1-1333-9792-7b75-fc310f5ce47e@archlinux.org","threadId":"56770","inReplyTo":"xmqqv91n57g4.fsf@gitster.g","subject":"Re: [PATCH 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-24T15:43:02Z","receivedAt":"2021-10-24T15:43:38Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/24/21 1:15 AM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> The %(describe) placeholder by default, like `git describe`, uses a\n>> seven-character abbreviated commit hash. This may not be sufficient to\n> \n> \"hash\" -> \"object name\".\n> \n>> fully describe all git repos, resulting in a placeholder replacement\n> \n> \"all git repos\" -> \"all commits in a given repository\" (there may be\n> other valid way to clarify, but the point is that 'describe' does\n> not describe 'git repos' in the sense that my repository gets\n> description X while your repository gets description Y).\n\n\nGood points.\n\n\n>> changing its length because the repository grew in size. This could\n>> cause the output of git-archive to change.\n>>\n>> Add the --abbrev option to `git describe` to the placeholder interface\n>> in order to provide tools to the user for fine-tuning project defaults\n>> and ensure reproducible archives.\n> \n> Note that it is sad that --abbrev= does not necessarily ensure\n> reproducibility. To be more precise, I do not think it sacrifices\n> uniqueness to make the output reproducible. You can get more than N\n> hex-digits in the output if N is too small to ensure uniquness.\n> \n> So it indeed is that this line of thought ...\n> \n>> One alternative would be to just always specify --abbrev=40 but this may\n>> be a bit too biased...\n> \n> ... to use --abbrev=999 (because 40 is not the length of a full\n> object name in the SHA-2 world) is the only reasonable way, if what\n> you care about is the reproducibility.\n\n\nRight, I keep forgetting about the current work towards SHA-2... that\nbeing said I somehow feel that 40 hex-digits will probably be reasonably\nsufficient even if commit object ids can become longer than that. So it\nis technically true...\n\n\n> Side note. I think \"git describe --no-abbrev\" is buggy in that\n> it does not give a full object name; I didn't check the code,\n> but it appears to be behaving the same way as \"git describe\n> --abbrev=0\" (show no hexdigits). Fixing this bug may possibly\n> be a low-hanging fruit.\n\n\nI... did not realize that --abbrev, which takes an integer value, could\nbe specified with a leading --no- in the first place. :o\n\n\n> But even if the feature cannot be used to guarantee a full\n> reproducibility, it is a good thing that we can now add this feature\n> with minimum effort thanks to the previous two steps.\n> \n> The refactoring I suggested in my review for the previous step will\n> shine, if we want to do a good job parsing the --abbrev= option,\n> since such a code organization would make it a fairly easy addition\n> to introduce \"integer\" type that calls match_placeholder_arg_value()\n> to read the option value (like \"string\" does) and validate that the\n> value is indeed an integer.\n> \n> Would we want to support \"--contains\" as another boolean type? How\n> about \"--all\" and \"--long\"? All three sound plausible candidates.\n\n\nI didn't have any immediate use for these options. To be honest, I don't\nentirely understand the purpose of:\n\n--all, which causes git describe to report things like \"heads/master\"\n\n--contains, which causes git describe to report different output\ndepending on the status of later commits being tagged\n\n\nThey may have their uses, but I'm not sure those uses include writing\nmetadata to a git-archive tarball at least. I believe the results would\ninevitably end up changing after the fact, whereas only matching\nexisting tags will tend to be pretty reliable as a tag is, in most\n(all?) cases, pushed at the same time as, or before:\n- the commit it describes\n- commits which are later than the tag\n\n\nOn the other hand...\n\n--long could be interesting to some people, although for, say,\ngenerating software version numbers, a tagged release will typically\nomit that information in my experience. For the sake of thoroughness I\ncould add that too.\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439619","messageId":"20211026013452.1372122-2-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211026013452.1372122-1-eschwartz@archlinux.org","subject":"[PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T01:34:50Z","receivedAt":"2021-10-26T01:36:15Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"It contains option arguments only, not options. We would like to add\noption support here too, but to do that we need to distinguish between\ndifferent types of options.\n\nLay out the groundwork for distinguishing between bools, strings, etc.\nand move the central logic (validating values and pushing new arguments\nto *args) into the successful match, because that will be fairly\nconditional on what type of argument is being parsed.\n\nSigned-off-by: Eli Schwartz \n---\n pretty.c | 29 +++++++++++++++++++----------\n 1 file changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..f8b254d2ff 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n-\tconst char *options[] = { \"match\", \"exclude\" };\n+\tstruct {\n+\t\tchar *name;\n+\t\tenum { OPT_STRING } type;\n+\t} option[] = {\n+\t\t{ \"exclude\", OPT_STRING },\n+\t\t{ \"match\", OPT_STRING },\n+\t};\n \tconst char *arg = start;\n \n \tfor (;;) {\n-\t\tconst char *matched = NULL;\n+\t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n \t\tint i;\n \n-\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n-\t\t\tif (match_placeholder_arg_value(arg, options[i], &arg,\n-\t\t\t\t\t\t\t&argval, &arglen)) {\n-\t\t\t\tmatched = options[i];\n+\t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n+\t\t\tswitch(option[i].type) {\n+\t\t\tcase OPT_STRING:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (!matched)\n+\t\tif (!found)\n \t\t\tbreak;\n \n-\t\tif (!arglen)\n-\t\t\treturn 0;\n-\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n \t}\n \treturn arg - start;\n }\n-- \n2.33.1\n\n"},{"id":"439620","messageId":"20211026013452.1372122-1-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211024014256.3569322-1-eschwartz@archlinux.org","subject":"[PATCH v2 0/3] Add some more options to the pretty-formats","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T01:34:49Z","receivedAt":"2021-10-26T01:36:15Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"Here is the reworked implementation and all-new replacement first patch,\nas suggested by review comments.\n\nMinor tweaks to documentation in the second patch, otherwise\ndocumentation and test cases are the same.\n\nEli Schwartz (3):\n pretty.c: rework describe options parsing for better extensibility\n pretty: add tag option to %(describe)\n pretty: add abbrev option to %(describe)\n\n Documentation/pretty-formats.txt | 16 +++++++---\n pretty.c | 55 ++++++++++++++++++++++++++------\n t/t4205-log-pretty-formats.sh | 16 ++++++++++\n 3 files changed, 72 insertions(+), 15 deletions(-)\n\n-- \n2.33.1\n\n"},{"id":"439621","messageId":"20211026013452.1372122-3-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211026013452.1372122-1-eschwartz@archlinux.org","subject":"[PATCH v2 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T01:34:51Z","receivedAt":"2021-10-26T01:36:15Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, only\nsupports annotated tags. However, some people do use lightweight tags\nfor releases, and would like to describe those anyway. The command line\ntool has an option to support this.\n\nTeach the placeholder to support this as well.\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 12 +++++++-----\n pretty.c | 14 +++++++++++++-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex ef6bd420ae..86ed801aad 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -220,6 +220,8 @@ The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n+** 'tags[=]': Instead of only considering annotated tags,\n+ consider lightweight tags as well.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n@@ -273,11 +275,6 @@ endif::git-rev-list[]\n \t\t\t If any option is provided multiple times the\n \t\t\t last occurrence wins.\n +\n-The boolean options accept an optional value `[=]`. The values\n-`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n-sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n-option is given with no value, it's enabled.\n-+\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n decoration format if `--decorate` was not already provided on the command\n line.\n \n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n+\n If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\ndiff --git a/pretty.c b/pretty.c\nindex f8b254d2ff..16b5366fed 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1218,8 +1218,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n \tstruct {\n \t\tchar *name;\n-\t\tenum { OPT_STRING } type;\n+\t\tenum { OPT_BOOL, OPT_STRING, } type;\n \t} option[] = {\n+\t\t{ \"tags\", OPT_BOOL},\n \t\t{ \"exclude\", OPT_STRING },\n \t\t{ \"match\", OPT_STRING },\n \t};\n@@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n+\t\tint optval = 0;\n \t\tint i;\n \n \t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n \t\t\tswitch(option[i].type) {\n+\t\t\tcase OPT_BOOL:\n+\t\t\t\tif(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n+\t\t\t\t\tif (optval) {\n+\t\t\t\t\t\tstrvec_pushf(args, \"--%s\", option[i].name);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tstrvec_pushf(args, \"--no-%s\", option[i].name);\n+\t\t\t\t\t}\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5865daa8f8..d4acf8882f 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:tags) vs git describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"439622","messageId":"20211026013452.1372122-4-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211026013452.1372122-1-eschwartz@archlinux.org","subject":"[PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T01:34:52Z","receivedAt":"2021-10-26T01:36:17Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, uses a\nseven-character abbreviated commit object name. This may not be\nsufficient to fully describe all commits in a given repository,\nresulting in a placeholder replacement changing its length because the\nrepository grew in size. This could cause the output of git-archive to\nchange.\n\nAdd the --abbrev option to `git describe` to the placeholder interface\nin order to provide tools to the user for fine-tuning project defaults\nand ensure reproducible archives.\n\nOne alternative would be to just always specify --abbrev=40 but this may\nbe a bit too biased...\n\nSigned-off-by: Eli Schwartz \n---\n\nNotes:\n With regard to validating that an integer is passed, I attempt to parse the\n result using the same mechanism git-describe itself does in the abbrev\n callback, just with slightly different validation of what we have at the end...\n because of course here argval is the entire rest of the format string,\n including the \")\".\n \n While testing that this actually does what it's supposed to do, I noticed that\n it doesn't validate junk like leading whitespace or plus signs... this is a\n problem for `git describe --abbrev=' +15'` too so I guess it's not my\n problem to fix...\n\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 16 +++++++++++++++-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 86ed801aad..57fd84f579 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -222,6 +222,10 @@ The placeholders are:\n +\n ** 'tags[=]': Instead of only considering annotated tags,\n consider lightweight tags as well.\n+** 'abbrev=': Instead of using the default number of hexadecimal digits\n+ (which will vary according to the number of objects in the repository with a\n+ default of 7) of the abbreviated object name, use digits, or as many digits\n+ as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\ndiff --git a/pretty.c b/pretty.c\nindex 16b5366fed..44bfc49b38 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n \tstruct {\n \t\tchar *name;\n-\t\tenum { OPT_BOOL, OPT_STRING, } type;\n+\t\tenum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;\n \t} option[] = {\n \t\t{ \"tags\", OPT_BOOL},\n+\t\t{ \"abbrev\", OPT_INTEGER },\n \t\t{ \"exclude\", OPT_STRING },\n \t\t{ \"match\", OPT_STRING },\n \t};\n@@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\t\t\t\tfound = 1;\n \t\t\t\t}\n \t\t\t\tbreak;\n+\t\t\tcase OPT_INTEGER:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tchar* endptr;\n+\t\t\t\t\tstrtol(argval, &endptr, 10);\n+\t\t\t\t\tif (endptr - argval != arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex d4acf8882f..35eef4c865 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag -a -m tagged tagname &&\n+\tgit describe --abbrev=15 >expect &&\n+\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"439625","messageId":"CAPig+cTWeN9_Z1jNLyyMsbRS4oOoyrPAWa3+JdCtsgE2B-rKFg@mail.gmail.com","threadId":"56770","inReplyTo":"20211026013452.1372122-2-eschwartz@archlinux.org","subject":"Re: [PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-26T05:18:03Z","receivedAt":"2021-10-26T05:18:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote:\n> It contains option arguments only, not options. We would like to add\n> option support here too, but to do that we need to distinguish between\n> different types of options.\n>\n> Lay out the groundwork for distinguishing between bools, strings, etc.\n> and move the central logic (validating values and pushing new arguments\n> to *args) into the successful match, because that will be fairly\n> conditional on what type of argument is being parsed.\n>\n> Signed-off-by: Eli Schwartz \n> ---\n> diff --git a/pretty.c b/pretty.c\n> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n> static size_t parse_describe_args(const char *start, struct strvec *args)\n> {\n> + struct {\n> + char *name;\n> + enum { OPT_STRING } type;\n> + } option[] = {\n> + { \"exclude\", OPT_STRING },\n> + { \"match\", OPT_STRING },\n> + };\n> const char *arg = start;\n>\n> for (;;) {\n> + int found = 0;\n> const char *argval;\n> size_t arglen = 0;\n> int i;\n>\n> + for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n> + switch(option[i].type) {\n> + case OPT_STRING:\n> + if (match_placeholder_arg_value(arg, option[i].name, &arg,\n> + &argval, &arglen) && arglen) {\n> + if (!arglen)\n> + return 0;\n\nI may be missing something obvious, but how will it be possible for:\n\n if (!arglen)\n return 0;\n\nto trigger if the `if` immediately above it:\n\n if (... && arglen) {\n\n has already asserted that `arglen` is not 0?\n\n> + strvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n> + found = 1;\n> + }\n> break;\n> }\n> }\n> + if (!found)\n> break;\n\nThe use of `found` to break out of a loop from within a `switch` seems\na bit clunky. An alternative would be to `goto` a label...\n\n> }\n> return arg - start;\n\n... which could be introduced just before the `return`. Of course,\nthis is highly subjective, so not necessarily worth changing.\n"},{"id":"439627","messageId":"CAPig+cTe9iMCteUYZZP_8cYoOzbg-95ptuVdvvk0SKUGMgrDjg@mail.gmail.com","threadId":"56770","inReplyTo":"20211026013452.1372122-3-eschwartz@archlinux.org","subject":"Re: [PATCH v2 2/3] pretty: add tag option to %(describe)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-26T05:25:07Z","receivedAt":"2021-10-26T05:25:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote:\n> The %(describe) placeholder by default, like `git describe`, only\n> supports annotated tags. However, some people do use lightweight tags\n> for releases, and would like to describe those anyway. The command line\n> tool has an option to support this.\n>\n> Teach the placeholder to support this as well.\n>\n> Signed-off-by: Eli Schwartz \n> ---\n> diff --git a/pretty.c b/pretty.c\n> @@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n> for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n> switch(option[i].type) {\n> + case OPT_BOOL:\n> + if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n\nStyle nit: add space after `if`\n\n> + if (optval) {\n> + strvec_pushf(args, \"--%s\", option[i].name);\n> + } else {\n> + strvec_pushf(args, \"--no-%s\", option[i].name);\n> + }\n\nWe would normally omit the braces for this simple `if`:\n\n if (optval)\n strvec_pushf(...);\n else\n strvec_pushf(...);\n\n... or maybe even use the ternary operator:\n\n strvec_pushf(args, \"--%s%s\", optval ? \"\" : \"no-\", option[i].name);\n\nbut it's highly subjective whether or not that's more readable.\n"},{"id":"439628","messageId":"CAPig+cT3u5uLq46Cyg3HZnGqy+UseHA-CN4dMDV1nYVyHkH-+Q@mail.gmail.com","threadId":"56770","inReplyTo":"20211026013452.1372122-4-eschwartz@archlinux.org","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-26T05:36:47Z","receivedAt":"2021-10-26T05:37:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote:\n> The %(describe) placeholder by default, like `git describe`, uses a\n> seven-character abbreviated commit object name. This may not be\n> sufficient to fully describe all commits in a given repository,\n> resulting in a placeholder replacement changing its length because the\n> repository grew in size. This could cause the output of git-archive to\n> change.\n>\n> Add the --abbrev option to `git describe` to the placeholder interface\n> in order to provide tools to the user for fine-tuning project defaults\n> and ensure reproducible archives.\n>\n> One alternative would be to just always specify --abbrev=40 but this may\n> be a bit too biased...\n>\n> Signed-off-by: Eli Schwartz \n> ---\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> @@ -222,6 +222,10 @@ The placeholders are:\n> +** 'abbrev=': Instead of using the default number of hexadecimal digits\n> + (which will vary according to the number of objects in the repository with a\n> + default of 7) of the abbreviated object name, use digits, or as many digits\n> + as needed to form a unique object name.\n\nInconsistent mix of `` and ``.\n\n> diff --git a/pretty.c b/pretty.c\n> @@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n> + case OPT_INTEGER:\n> + if (match_placeholder_arg_value(arg, option[i].name, &arg,\n> + &argval, &arglen) && arglen) {\n> + if (!arglen)\n> + return 0;\n\nSame question I asked while reviewing the other patch regarding\nchecking `arglen` in both conditionals: `if (... && arglen)` vs. `if\n(!arglen)`\n"},{"id":"439638","messageId":"YXfvY3n9wEwctjUR@danh.dev","threadId":"56770","inReplyTo":"20211026013452.1372122-4-eschwartz@archlinux.org","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-10-26T12:06:59Z","receivedAt":"2021-10-26T12:07:05Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-10-25 21:34:52-0400, Eli Schwartz wrote:\n> The %(describe) placeholder by default, like `git describe`, uses a\n> seven-character abbreviated commit object name. This may not be\n> sufficient to fully describe all commits in a given repository,\n> resulting in a placeholder replacement changing its length because the\n> repository grew in size. This could cause the output of git-archive to\n> change.\n> \n> Add the --abbrev option to `git describe` to the placeholder interface\n> in order to provide tools to the user for fine-tuning project defaults\n> and ensure reproducible archives.\n> \n> One alternative would be to just always specify --abbrev=40 but this may\n> be a bit too biased...\n> \n> Signed-off-by: Eli Schwartz \n> ---\n> \n> Notes:\n> With regard to validating that an integer is passed, I attempt to parse the\n> result using the same mechanism git-describe itself does in the abbrev\n> callback, just with slightly different validation of what we have at the end...\n> because of course here argval is the entire rest of the format string,\n> including the \")\".\n> \n> While testing that this actually does what it's supposed to do, I noticed that\n> it doesn't validate junk like leading whitespace or plus signs... this is a\n> problem for `git describe --abbrev=' +15'` too so I guess it's not my\n> problem to fix...\n> \n> Documentation/pretty-formats.txt | 4 ++++\n> pretty.c | 16 +++++++++++++++-\n> t/t4205-log-pretty-formats.sh | 8 ++++++++\n> 3 files changed, 27 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 86ed801aad..57fd84f579 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -222,6 +222,10 @@ The placeholders are:\n> +\n> ** 'tags[=]': Instead of only considering annotated tags,\n> consider lightweight tags as well.\n> +** 'abbrev=': Instead of using the default number of hexadecimal digits\n> + (which will vary according to the number of objects in the repository with a\n> + default of 7) of the abbreviated object name, use digits, or as many digits\n> + as needed to form a unique object name.\n> ** 'match=': Only consider tags matching the given\n> `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n> ** 'exclude=': Do not consider tags matching the given\n> diff --git a/pretty.c b/pretty.c\n> index 16b5366fed..44bfc49b38 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n> {\n> \tstruct {\n> \t\tchar *name;\n> -\t\tenum { OPT_BOOL, OPT_STRING, } type;\n> +\t\tenum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;\n> \t} option[] = {\n> \t\t{ \"tags\", OPT_BOOL},\n> +\t\t{ \"abbrev\", OPT_INTEGER },\n> \t\t{ \"exclude\", OPT_STRING },\n> \t\t{ \"match\", OPT_STRING },\n> \t};\n> @@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n> \t\t\t\t\tfound = 1;\n> \t\t\t\t}\n> \t\t\t\tbreak;\n> +\t\t\tcase OPT_INTEGER:\n> +\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n> +\t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n> +\t\t\t\t\tif (!arglen)\n> +\t\t\t\t\t\treturn 0;\n> +\t\t\t\t\tchar* endptr;\n\nOther than the question pointed out by Eric,\n\nwith DEVELOPER=1, -Werror=declaration-after-statement\nWe'll need this change squashed in:\n\n------- 8< -----\ndiff --git a/pretty.c b/pretty.c\nindex 289b5456c8..85d4ab008b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1249,9 +1249,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\t\tcase OPT_INTEGER:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n+\t\t\t\t\tchar* endptr;\n \t\t\t\t\tif (!arglen)\n \t\t\t\t\t\treturn 0;\n-\t\t\t\t\tchar* endptr;\n \t\t\t\t\tstrtol(argval, &endptr, 10);\n \t\t\t\t\tif (endptr - argval != arglen)\n \t\t\t\t\t\treturn 0;\n------- >8 -----\n\n> +\t\t\t\t\tstrtol(argval, &endptr, 10);\n> +\t\t\t\t\tif (endptr - argval != arglen)\n> +\t\t\t\t\t\treturn 0;\n> +\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n> +\t\t\t\t\tfound = 1;\n> +\t\t\t\t}\n> +\t\t\t\tbreak;\n> \t\t\tcase OPT_STRING:\n> \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n> \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index d4acf8882f..35eef4c865 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n> +\ttest_when_finished \"git tag -d tagname\" &&\n> +\tgit tag -a -m tagged tagname &&\n> +\tgit describe --abbrev=15 >expect &&\n> +\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 2.33.1\n> \n\n-- \nDanh\n"},{"id":"439661","messageId":"CAPig+cQoNJpzpL_DYiDFFbZzKwRbwJzuS4+fUcwZ_OLx8onHtA@mail.gmail.com","threadId":"56770","inReplyTo":"YXfvY3n9wEwctjUR@danh.dev","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-26T17:28:50Z","receivedAt":"2021-10-26T17:29:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 26, 2021 at 8:07 AM Đoàn Trần Công Danh\n wrote:\n> On 2021-10-25 21:34:52-0400, Eli Schwartz wrote:\n> > + if (!arglen)\n> > + return 0;\n> > + char* endptr;\n>\n> Other than the question pointed out by Eric,\n>\n> with DEVELOPER=1, -Werror=declaration-after-statement\n> We'll need this change squashed in:\n>\n> + char* endptr;\n> if (!arglen)\n> return 0;\n> - char* endptr;\n\nThis highlights a style nit, as well; should be:\n\n char *endptr;\n"},{"id":"439666","messageId":"43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org","threadId":"56770","inReplyTo":"YXfvY3n9wEwctjUR@danh.dev","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T19:12:43Z","receivedAt":"2021-10-26T19:12:51Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:\n> Other than the question pointed out by Eric,\n> \n> with DEVELOPER=1, -Werror=declaration-after-statement\n> We'll need this change squashed in:\n\n\nThanks for the advice. In v1 of this patchset I attempted to do a\ndeveloper build but failed due to preexisting errors:\n\n\n CC run-command.o\nrun-command.c: In function ‘async_die_is_recursing’:\nrun-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a\nregion of size 0 [-Werror=stringop-overread]\n 1102 | pthread_setspecific(async_die_counter, (void *)1);\n | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nIn file included from /usr/include/openssl/crypto.h:415,\n from /usr/include/openssl/comp.h:16,\n from /usr/include/openssl/ssl.h:17,\n from git-compat-util.h:309,\n from cache.h:4,\n from run-command.c:1:\n/usr/include/pthread.h:1308:12: note: in a call to function\n‘pthread_setspecific’ declared with attribute ‘access (none, 2)’\n 1308 | extern int pthread_setspecific (pthread_key_t __key,\n | ^~~~~~~~~~~~~~~~~~~\ncc1: all warnings being treated as errors\n\n\n\nMy system has a custom compiled glibc from git roughly around the 2.34\nrelease (a similar environment could be obtained by using Fedora rawhide\nI guess), and this commit looks mighty suspicious:\nhttps://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0\n\nFor this reason, I did not bother to try testing v2 under a developer\nbuild, leading to my overlooking this issue. ;)\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439669","messageId":"7bd2ef6f-9afe-2c44-db51-9307b0cd5f0f@archlinux.org","threadId":"56770","inReplyTo":"CAPig+cTWeN9_Z1jNLyyMsbRS4oOoyrPAWa3+JdCtsgE2B-rKFg@mail.gmail.com","subject":"Re: [PATCH v2 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T20:05:59Z","receivedAt":"2021-10-26T20:06:07Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/26/21 1:18 AM, Eric Sunshine wrote:\n> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote:\n>> It contains option arguments only, not options. We would like to add\n>> option support here too, but to do that we need to distinguish between\n>> different types of options.\n>>\n>> Lay out the groundwork for distinguishing between bools, strings, etc.\n>> and move the central logic (validating values and pushing new arguments\n>> to *args) into the successful match, because that will be fairly\n>> conditional on what type of argument is being parsed.\n>>\n>> Signed-off-by: Eli Schwartz \n>> ---\n>> diff --git a/pretty.c b/pretty.c\n>> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n>> static size_t parse_describe_args(const char *start, struct strvec *args)\n>> {\n>> + struct {\n>> + char *name;\n>> + enum { OPT_STRING } type;\n>> + } option[] = {\n>> + { \"exclude\", OPT_STRING },\n>> + { \"match\", OPT_STRING },\n>> + };\n>> const char *arg = start;\n>>\n>> for (;;) {\n>> + int found = 0;\n>> const char *argval;\n>> size_t arglen = 0;\n>> int i;\n>>\n>> + for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n>> + switch(option[i].type) {\n>> + case OPT_STRING:\n>> + if (match_placeholder_arg_value(arg, option[i].name, &arg,\n>> + &argval, &arglen) && arglen) {\n>> + if (!arglen)\n>> + return 0;\n> \n> I may be missing something obvious, but how will it be possible for:\n> \n> if (!arglen)\n> return 0;\n> \n> to trigger if the `if` immediately above it:\n> \n> if (... && arglen) {\n> \n> has already asserted that `arglen` is not 0?\n\n\nI don't think you are missing anything here, I simply forgot that\nhalfway through I added a second check to the if, and later moved the\ncode from down below.\n\nI think returning 0 is correct here, to avoid pointlessly checking the\nrest of option[]. So I'll (re-)remove the first check.\n\n\n>> + strvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n>> + found = 1;\n>> + }\n>> break;\n>> }\n>> }\n>> + if (!found)\n>> break;\n> \n> The use of `found` to break out of a loop from within a `switch` seems\n> a bit clunky. An alternative would be to `goto` a label...\n> \n>> }\n>> return arg - start;\n> \n> ... which could be introduced just before the `return`. Of course,\n> this is highly subjective, so not necessarily worth changing.\n\n\nKeeping in mind that this for (;;) { .... break; } was there before me\n:D I just switched the name/type of the variable it checks...\n\nIMO changing to goto is not my business to change (at least not in this\npatch), and given the \"common wisdom\" is \"goto is evil\" I'm not strongly\ninclined to get into the business of rewriting someone else's code for\nthat. It's too subjective for my taste.\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439670","messageId":"5e2233d8-f63c-d786-ee9f-31865d6c208c@archlinux.org","threadId":"56770","inReplyTo":"CAPig+cTe9iMCteUYZZP_8cYoOzbg-95ptuVdvvk0SKUGMgrDjg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-26T20:06:25Z","receivedAt":"2021-10-26T20:06:31Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/26/21 1:25 AM, Eric Sunshine wrote:\n> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz wrote:\n>> The %(describe) placeholder by default, like `git describe`, only\n>> supports annotated tags. However, some people do use lightweight tags\n>> for releases, and would like to describe those anyway. The command line\n>> tool has an option to support this.\n>>\n>> Teach the placeholder to support this as well.\n>>\n>> Signed-off-by: Eli Schwartz \n>> ---\n>> diff --git a/pretty.c b/pretty.c\n>> @@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n>> for (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n>> switch(option[i].type) {\n>> + case OPT_BOOL:\n>> + if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n> \n> Style nit: add space after `if`\n\n\nOops, I am not sure how this happened. It's wrong in the switch too.\n\n\n>> + if (optval) {\n>> + strvec_pushf(args, \"--%s\", option[i].name);\n>> + } else {\n>> + strvec_pushf(args, \"--no-%s\", option[i].name);\n>> + }\n> \n> We would normally omit the braces for this simple `if`:\n> \n> if (optval)\n> strvec_pushf(...);\n> else\n> strvec_pushf(...);\n> \n> ... or maybe even use the ternary operator:\n> \n> strvec_pushf(args, \"--%s%s\", optval ? \"\" : \"no-\", option[i].name);\n> \n> but it's highly subjective whether or not that's more readable.\n\n\nAlthough the braces feel more natural to me for clarity purposes, it's a\ngood point that the git coding style says to omit them for single\nstatements, and I should have followed that here.\n\nThe ternary doesn't feel readable to me, however.\n\n...\n\nThanks for the style review!\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"439719","messageId":"CAPUEsph_03V-1-Euj_9P1PAA9EBZHN+utEV9TvfQxwb2QTPdag@mail.gmail.com","threadId":"56770","inReplyTo":"43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-27T08:05:17Z","receivedAt":"2021-10-27T08:05:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Oct 27, 2021 at 12:23 AM Eli Schwartz wrote:\n>\n> On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:\n> > Other than the question pointed out by Eric,\n> >\n> > with DEVELOPER=1, -Werror=declaration-after-statement\n> > We'll need this change squashed in:\n>\n> Thanks for the advice. In v1 of this patchset I attempted to do a\n> developer build but failed due to preexisting errors:\n\nyou can always avoid breaking your build by using:\n\n DEVOPTS=no-error\n\nCarlo\n"},{"id":"440042","messageId":"20211029184512.1568017-1-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211026013452.1372122-1-eschwartz@archlinux.org","subject":"[PATCH v3 0/3] Add some more options to the pretty-formats","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T18:45:09Z","receivedAt":"2021-10-29T18:45:36Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"This revision only contains style nits in response to review comments.\nSee below.\n\nEli Schwartz (3):\n pretty.c: rework describe options parsing for better extensibility\n pretty: add tag option to %(describe)\n pretty: add abbrev option to %(describe)\n\n Documentation/pretty-formats.txt | 16 +++++++---\n pretty.c | 54 ++++++++++++++++++++++++++------\n t/t4205-log-pretty-formats.sh | 16 ++++++++++\n 3 files changed, 71 insertions(+), 15 deletions(-)\n\nRange-diff against v2:\n1: 1cf0d82b91 ! 1: 55a20468d3 pretty.c: rework describe options parsing for better extensibility\n @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,\n -\t\t\t\t\t\t\t&argval, &arglen)) {\n -\t\t\t\tmatched = options[i];\n +\t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n -+\t\t\tswitch(option[i].type) {\n ++\t\t\tswitch (option[i].type) {\n +\t\t\tcase OPT_STRING:\n +\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n -+\t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n ++\t\t\t\t\t\t\t\t&argval, &arglen)) {\n +\t\t\t\t\tif (!arglen)\n +\t\t\t\t\t\treturn 0;\n +\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n2: cb6af9bc14 ! 2: c34c8a4f7f pretty: add tag option to %(describe)\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n \t\tint i;\n \n \t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n - \t\t\tswitch(option[i].type) {\n + \t\t\tswitch (option[i].type) {\n +\t\t\tcase OPT_BOOL:\n -+\t\t\t\tif(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n -+\t\t\t\t\tif (optval) {\n ++\t\t\t\tif (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n ++\t\t\t\t\tif (optval)\n +\t\t\t\t\t\tstrvec_pushf(args, \"--%s\", option[i].name);\n -+\t\t\t\t\t} else {\n ++\t\t\t\t\telse\n +\t\t\t\t\t\tstrvec_pushf(args, \"--no-%s\", option[i].name);\n -+\t\t\t\t\t}\n +\t\t\t\t\tfound = 1;\n +\t\t\t\t}\n +\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n - \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n + \t\t\t\t\t\t\t\t&argval, &arglen)) {\n \n ## t/t4205-log-pretty-formats.sh ##\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n3: 08ade18b35 ! 3: b751aaf3c6 pretty: add abbrev option to %(describe)\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n \t\t\t\tbreak;\n +\t\t\tcase OPT_INTEGER:\n +\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n -+\t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n ++\t\t\t\t\t\t\t\t&argval, &arglen)) {\n ++\t\t\t\t\tchar *endptr;\n +\t\t\t\t\tif (!arglen)\n +\t\t\t\t\t\treturn 0;\n -+\t\t\t\t\tchar* endptr;\n +\t\t\t\t\tstrtol(argval, &endptr, 10);\n +\t\t\t\t\tif (endptr - argval != arglen)\n +\t\t\t\t\t\treturn 0;\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n +\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n - \t\t\t\t\t\t\t\t&argval, &arglen) && arglen) {\n + \t\t\t\t\t\t\t\t&argval, &arglen)) {\n \n ## t/t4205-log-pretty-formats.sh ##\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(describe:tags) vs git describe --tags' '\n-- \n2.33.1\n\n"},{"id":"440043","messageId":"20211029184512.1568017-3-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211029184512.1568017-1-eschwartz@archlinux.org","subject":"[PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T18:45:11Z","receivedAt":"2021-10-29T18:45:43Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, only\nsupports annotated tags. However, some people do use lightweight tags\nfor releases, and would like to describe those anyway. The command line\ntool has an option to support this.\n\nTeach the placeholder to support this as well.\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 12 +++++++-----\n pretty.c | 13 ++++++++++++-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex ef6bd420ae..86ed801aad 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -220,6 +220,8 @@ The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n+** 'tags[=]': Instead of only considering annotated tags,\n+ consider lightweight tags as well.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n@@ -273,11 +275,6 @@ endif::git-rev-list[]\n \t\t\t If any option is provided multiple times the\n \t\t\t last occurrence wins.\n +\n-The boolean options accept an optional value `[=]`. The values\n-`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n-sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n-option is given with no value, it's enabled.\n-+\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n decoration format if `--decorate` was not already provided on the command\n line.\n \n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n+\n If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\ndiff --git a/pretty.c b/pretty.c\nindex 2ec023a0d0..a105ef2a15 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1218,8 +1218,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n \tstruct {\n \t\tchar *name;\n-\t\tenum { OPT_STRING } type;\n+\t\tenum { OPT_BOOL, OPT_STRING, } type;\n \t} option[] = {\n+\t\t{ \"tags\", OPT_BOOL},\n \t\t{ \"exclude\", OPT_STRING },\n \t\t{ \"match\", OPT_STRING },\n \t};\n@@ -1229,10 +1230,20 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n+\t\tint optval = 0;\n \t\tint i;\n \n \t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n \t\t\tswitch (option[i].type) {\n+\t\t\tcase OPT_BOOL:\n+\t\t\t\tif (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n+\t\t\t\t\tif (optval)\n+\t\t\t\t\t\tstrvec_pushf(args, \"--%s\", option[i].name);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrvec_pushf(args, \"--no-%s\", option[i].name);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5865daa8f8..d4acf8882f 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:tags) vs git describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440044","messageId":"20211029184512.1568017-4-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211029184512.1568017-1-eschwartz@archlinux.org","subject":"[PATCH v3 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T18:45:12Z","receivedAt":"2021-10-29T18:45:46Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, uses a\nseven-character abbreviated commit object name. This may not be\nsufficient to fully describe all commits in a given repository,\nresulting in a placeholder replacement changing its length because the\nrepository grew in size. This could cause the output of git-archive to\nchange.\n\nAdd the --abbrev option to `git describe` to the placeholder interface\nin order to provide tools to the user for fine-tuning project defaults\nand ensure reproducible archives.\n\nOne alternative would be to just always specify --abbrev=40 but this may\nbe a bit too biased...\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 16 +++++++++++++++-\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 86ed801aad..57fd84f579 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -222,6 +222,10 @@ The placeholders are:\n +\n ** 'tags[=]': Instead of only considering annotated tags,\n consider lightweight tags as well.\n+** 'abbrev=': Instead of using the default number of hexadecimal digits\n+ (which will vary according to the number of objects in the repository with a\n+ default of 7) of the abbreviated object name, use digits, or as many digits\n+ as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\ndiff --git a/pretty.c b/pretty.c\nindex a105ef2a15..5662cb2943 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n \tstruct {\n \t\tchar *name;\n-\t\tenum { OPT_BOOL, OPT_STRING, } type;\n+\t\tenum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;\n \t} option[] = {\n \t\t{ \"tags\", OPT_BOOL},\n+\t\t{ \"abbrev\", OPT_INTEGER },\n \t\t{ \"exclude\", OPT_STRING },\n \t\t{ \"match\", OPT_STRING },\n \t};\n@@ -1244,6 +1245,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\t\t\t\tfound = 1;\n \t\t\t\t}\n \t\t\t\tbreak;\n+\t\t\tcase OPT_INTEGER:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen)) {\n+\t\t\t\t\tchar *endptr;\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrtol(argval, &endptr, 10);\n+\t\t\t\t\tif (endptr - argval != arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase OPT_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex d4acf8882f..35eef4c865 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag -a -m tagged tagname &&\n+\tgit describe --abbrev=15 >expect &&\n+\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440045","messageId":"20211029184512.1568017-2-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211029184512.1568017-1-eschwartz@archlinux.org","subject":"[PATCH v3 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T18:45:10Z","receivedAt":"2021-10-29T18:45:48Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"It contains option arguments only, not options. We would like to add\noption support here too, but to do that we need to distinguish between\ndifferent types of options.\n\nLay out the groundwork for distinguishing between bools, strings, etc.\nand move the central logic (validating values and pushing new arguments\nto *args) into the successful match, because that will be fairly\nconditional on what type of argument is being parsed.\n\nSigned-off-by: Eli Schwartz \n---\n pretty.c | 29 +++++++++++++++++++----------\n 1 file changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex fe95107ae5..2ec023a0d0 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n-\tconst char *options[] = { \"match\", \"exclude\" };\n+\tstruct {\n+\t\tchar *name;\n+\t\tenum { OPT_STRING } type;\n+\t} option[] = {\n+\t\t{ \"exclude\", OPT_STRING },\n+\t\t{ \"match\", OPT_STRING },\n+\t};\n \tconst char *arg = start;\n \n \tfor (;;) {\n-\t\tconst char *matched = NULL;\n+\t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n \t\tint i;\n \n-\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n-\t\t\tif (match_placeholder_arg_value(arg, options[i], &arg,\n-\t\t\t\t\t\t\t&argval, &arglen)) {\n-\t\t\t\tmatched = options[i];\n+\t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n+\t\t\tswitch (option[i].type) {\n+\t\t\tcase OPT_STRING:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen)) {\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (!matched)\n+\t\tif (!found)\n \t\t\tbreak;\n \n-\t\tif (!arglen)\n-\t\t\treturn 0;\n-\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n \t}\n \treturn arg - start;\n }\n-- \n2.33.1\n\n"},{"id":"440048","messageId":"CAPig+cSvecU9XSVobxSO-72rFgAMh5D39UcS6SJ2=xFVvnGJBA@mail.gmail.com","threadId":"56770","inReplyTo":"20211029184512.1568017-4-eschwartz@archlinux.org","subject":"Re: [PATCH v3 3/3] pretty: add abbrev option to %(describe)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-29T18:51:48Z","receivedAt":"2021-10-29T18:52:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 29, 2021 at 2:45 PM Eli Schwartz wrote:\n> The %(describe) placeholder by default, like `git describe`, uses a\n> seven-character abbreviated commit object name. This may not be\n> sufficient to fully describe all commits in a given repository,\n> resulting in a placeholder replacement changing its length because the\n> repository grew in size. This could cause the output of git-archive to\n> change.\n>\n> Add the --abbrev option to `git describe` to the placeholder interface\n> in order to provide tools to the user for fine-tuning project defaults\n> and ensure reproducible archives.\n> [...]\n> Signed-off-by: Eli Schwartz \n> ---\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> @@ -222,6 +222,10 @@ The placeholders are:\n> +** 'abbrev=': Instead of using the default number of hexadecimal digits\n> + (which will vary according to the number of objects in the repository with a\n> + default of 7) of the abbreviated object name, use digits, or as many digits\n> + as needed to form a unique object name.\n\nThere's still an inconsistent mix of `` and `` here (mentioned\nin my earlier review). Is that intentional or just a simple oversight?\n"},{"id":"440050","messageId":"3c3cc160-fe7a-30c1-d65e-209dcc07d76e@archlinux.org","threadId":"56770","inReplyTo":"CAPig+cSvecU9XSVobxSO-72rFgAMh5D39UcS6SJ2=xFVvnGJBA@mail.gmail.com","subject":"Re: [PATCH v3 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T19:04:30Z","receivedAt":"2021-10-29T19:04:38Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/29/21 2:51 PM, Eric Sunshine wrote:\n> On Fri, Oct 29, 2021 at 2:45 PM Eli Schwartz wrote:\n>> The %(describe) placeholder by default, like `git describe`, uses a\n>> seven-character abbreviated commit object name. This may not be\n>> sufficient to fully describe all commits in a given repository,\n>> resulting in a placeholder replacement changing its length because the\n>> repository grew in size. This could cause the output of git-archive to\n>> change.\n>>\n>> Add the --abbrev option to `git describe` to the placeholder interface\n>> in order to provide tools to the user for fine-tuning project defaults\n>> and ensure reproducible archives.\n>> [...]\n>> Signed-off-by: Eli Schwartz \n>> ---\n>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n>> @@ -222,6 +222,10 @@ The placeholders are:\n>> +** 'abbrev=': Instead of using the default number of hexadecimal digits\n>> + (which will vary according to the number of objects in the repository with a\n>> + default of 7) of the abbreviated object name, use digits, or as many digits\n>> + as needed to form a unique object name.\n> \n> There's still an inconsistent mix of `` and `` here (mentioned\n> in my earlier review). Is that intentional or just a simple oversight?\n\n\nAh, sorry... I overlooked that. It was originally copied from the\ngit-describe man page which uses lowercase and I overlooked that part of\nyour review.\n\nIt should be consistently uppercase here for consistency with\npretty-formats.\n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"440056","messageId":"xmqq35ojlhg6.fsf@gitster.g","threadId":"56770","inReplyTo":"20211029184512.1568017-2-eschwartz@archlinux.org","subject":"Re: [PATCH v3 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:11:21Z","receivedAt":"2021-10-29T20:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> +\tstruct {\n> +\t\tchar *name;\n> +\t\tenum { OPT_STRING } type;\n> +\t} option[] = {\n> +\t\t{ \"exclude\", OPT_STRING },\n> +\t\t{ \"match\", OPT_STRING },\n> +\t};\n\nI floated OPT_ in my earlier illustration as \"something like\nthis\", not \"literally use these tokens\". We have CPP macros of the\nsame name in parse-options.h API---we may not see troubles from the\nname clashes today, but let's not leave it to chances.\n\nPerhaps call it like DESCRBE_ARG_STRING or something that ensures\nuniqueness like that?\n\nThanks.\n"},{"id":"440058","messageId":"xmqqy26bk2k9.fsf@gitster.g","threadId":"56770","inReplyTo":"20211029184512.1568017-3-eschwartz@archlinux.org","subject":"Re: [PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T20:18:14Z","receivedAt":"2021-10-29T20:18:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> +\n> +** 'tags[=]': Instead of only considering annotated tags,\n> + consider lightweight tags as well.\n\nThis part contradicts what Jean-Noël's df34a41f is trying to\nachieve, which can be seen in these hunks from it:\n\n @@ -273,12 +273,12 @@ endif::git-rev-list[]\n If any option is provided multiple times the\n last occurrence wins.\n +\n -The boolean options accept an optional value `[=]`. The values\n +The boolean options accept an optional value `[=]`. The values\n `true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n option is given with no value, it's enabled.\n +\n -** 'key=': only show trailers with specified key. Matching is done\n +** 'key=': only show trailers with specified . Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n shown. This option automatically enables the `only` option so that\n @@ -286,25 +286,25 @@ option is given with no value, it's enabled.\n desired it can be disabled with `only=false`. E.g.,\n `%(trailers:key=Reviewed-by)` shows trailer lines with key\n `Reviewed-by`.\n -** 'only[=]': select whether non-trailer lines from the trailer\n +** 'only[=]': select whether non-trailer lines from the trailer\n block should be included.\n -** 'separator=': specify a separator inserted between trailer\n +** 'separator=': specify a separator inserted between trailer\n ...\n\n\nSo, let's instead use\n\n tags[=]: Instead of only considering ...\n\ni.e. lowercase, with -value suffix.\n\nThanks.\n"},{"id":"440069","messageId":"aef5409c-384e-1010-9f33-e3bfe1aa0685@archlinux.org","threadId":"56770","inReplyTo":"xmqq35ojlhg6.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T21:06:56Z","receivedAt":"2021-10-29T21:07:04Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/29/21 4:11 PM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> +\tstruct {\n>> +\t\tchar *name;\n>> +\t\tenum { OPT_STRING } type;\n>> +\t} option[] = {\n>> +\t\t{ \"exclude\", OPT_STRING },\n>> +\t\t{ \"match\", OPT_STRING },\n>> +\t};\n> \n> I floated OPT_ in my earlier illustration as \"something like\n> this\", not \"literally use these tokens\". We have CPP macros of the\n> same name in parse-options.h API---we may not see troubles from the\n> name clashes today, but let's not leave it to chances.\n> \n> Perhaps call it like DESCRBE_ARG_STRING or something that ensures\n> uniqueness like that?\n\n\nAh. That alternative seems a bit long though. :( Without breaking enum\ntype into one per line, it will quickly overflow line lengths... though\nmaybe it should be one per line anyway?\n\nWill try to put some thought into naming.\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"440073","messageId":"3cf891ea-58c8-b7d5-0b6e-eb23dff92bd5@archlinux.org","threadId":"56770","inReplyTo":"xmqqy26bk2k9.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T21:14:15Z","receivedAt":"2021-10-29T21:14:23Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/29/21 4:18 PM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> +\n>> +** 'tags[=]': Instead of only considering annotated tags,\n>> + consider lightweight tags as well.\n> \n> This part contradicts what Jean-Noël's df34a41f is trying to\n> achieve, which can be seen in these hunks from it:\n>\n> [...]\n> \n> So, let's instead use\n> \n> tags[=]: Instead of only considering ...\n> \n> i.e. lowercase, with -value suffix.\n\n\nAn interesting change. I can use that description style, sure. Though I\nwill note the commit message for it talks a lot about replacing spaces\nwith hyphens, and very little about consolidating on case *or* using\ndifferent language such as:\n\n\n-* 'format:'\n+* 'format:'\n\n\nI also assume that it's fine for my patches to be inconsistent with the\nbase commit, as it's expected df34a41f or some revision of it will be\nmerged around the same time?\n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"440080","messageId":"xmqq7ddvjzbs.fsf@gitster.g","threadId":"56770","inReplyTo":"xmqqy26bk2k9.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T21:28:07Z","receivedAt":"2021-10-29T21:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eli Schwartz writes:\n>\n>> +\n>> +** 'tags[=]': Instead of only considering annotated tags,\n>> + consider lightweight tags as well.\n>\n> This part contradicts what Jean-Noël's df34a41f is trying to\n> achieve, which can be seen in these hunks from it:\n> ...\n> So, let's instead use\n>\n> tags[=]: Instead of only considering ...\n>\n> i.e. lowercase, with -value suffix.\n\nThe other topic merges earlier to 'seen' before your topic, and FYI,\nthe diff between the tip of 'seen' before and after your topic gets\nmerged looks like this, with my semantic conflict resolution.\n\nNotice the way placeholders are spelled in lowercase and generally\nhave more descriptive names.\n\nThanks.\n\ndiff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt\nindex d465cd59dd..25cfffab38 100644\n--- c/Documentation/pretty-formats.txt\n+++ w/Documentation/pretty-formats.txt\n@@ -220,6 +220,12 @@ The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n+** 'tags[=]': Instead of only considering annotated tags,\n+ consider lightweight tags as well.\n+** 'abbrev=': Instead of using the default number of hexadecimal digits\n+ (which will vary according to the number of objects in the repository with a\n+ default of 7) of the abbreviated object name, use digits, or as many digits\n+ as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n@@ -273,11 +279,6 @@ endif::git-rev-list[]\n \t\t\t If any option is provided multiple times the\n \t\t\t last occurrence wins.\n +\n-The boolean options accept an optional value `[=]`. The values\n-`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n-sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n-option is given with no value, it's enabled.\n-+\n ** 'key=': only show trailers with specified . Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n@@ -313,6 +314,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n decoration format if `--decorate` was not already provided on the command\n line.\n \n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n+\n If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\n"},{"id":"440082","messageId":"xmqq35ojjz12.fsf@gitster.g","threadId":"56770","inReplyTo":"aef5409c-384e-1010-9f33-e3bfe1aa0685@archlinux.org","subject":"Re: [PATCH v3 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T21:34:33Z","receivedAt":"2021-10-29T21:34:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> On 10/29/21 4:11 PM, Junio C Hamano wrote:\n>> Eli Schwartz writes:\n>> \n>>> +\tstruct {\n>>> +\t\tchar *name;\n>>> +\t\tenum { OPT_STRING } type;\n>>> +\t} option[] = {\n>>> +\t\t{ \"exclude\", OPT_STRING },\n>>> +\t\t{ \"match\", OPT_STRING },\n>>> +\t};\n>> \n>> I floated OPT_ in my earlier illustration as \"something like\n>> this\", not \"literally use these tokens\". We have CPP macros of the\n>> same name in parse-options.h API---we may not see troubles from the\n>> name clashes today, but let's not leave it to chances.\n>> \n>> Perhaps call it like DESCRBE_ARG_STRING or something that ensures\n>> uniqueness like that?\n>\n>\n> Ah. That alternative seems a bit long though. :( Without breaking enum\n> type into one per line, it will quickly overflow line lengths... though\n> maybe it should be one per line anyway?\n\nYes, these things should be one item per line; a patch that adds or\nremoves one would become easier to read.\n\n>\n> Will try to put some thought into naming.\n"},{"id":"440088","messageId":"ab609944-d4ec-cf50-1a2f-c0e159a541d5@archlinux.org","threadId":"56770","inReplyTo":"xmqq7ddvjzbs.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-29T21:44:23Z","receivedAt":"2021-10-29T21:44:34Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/29/21 5:28 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Eli Schwartz writes:\n>>\n>>> +\n>>> +** 'tags[=]': Instead of only considering annotated tags,\n>>> + consider lightweight tags as well.\n>>\n>> This part contradicts what Jean-Noël's df34a41f is trying to\n>> achieve, which can be seen in these hunks from it:\n>> ...\n>> So, let's instead use\n>>\n>> tags[=]: Instead of only considering ...\n>>\n>> i.e. lowercase, with -value suffix.\n> \n> The other topic merges earlier to 'seen' before your topic, and FYI,\n> the diff between the tip of 'seen' before and after your topic gets\n> merged looks like this, with my semantic conflict resolution.\n> \n> Notice the way placeholders are spelled in lowercase and generally\n> have more descriptive names.\n> \n> Thanks.\n> \n> diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt\n> index d465cd59dd..25cfffab38 100644\n> --- c/Documentation/pretty-formats.txt\n> +++ w/Documentation/pretty-formats.txt\n> @@ -220,6 +220,12 @@ The placeholders are:\n> \t\t\t inconsistent when tags are added or removed at\n> \t\t\t the same time.\n> +\n> +** 'tags[=]': Instead of only considering annotated tags,\n> + consider lightweight tags as well.\n> +** 'abbrev=': Instead of using the default number of hexadecimal digits\n\n\nAs a matter of curiosity, why \"bool-value\" but not \"number-value\"?\n\nIsn't the \"value\" part implicit?\n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"440089","messageId":"xmqqsfwjijwl.fsf@gitster.g","threadId":"56770","inReplyTo":"3cf891ea-58c8-b7d5-0b6e-eb23dff92bd5@archlinux.org","subject":"Re: [PATCH v3 2/3] pretty: add tag option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-29T21:46:34Z","receivedAt":"2021-10-29T21:46:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> I also assume that it's fine for my patches to be inconsistent with the\n> base commit, as it's expected df34a41f or some revision of it will be\n> merged around the same time?\n\nYes, such inconsistencies will be gone when the both topics get\nmerged. You can just assume that the details of what you write may\nnot matter in the end result ;-)\n\nOr perhaps the other topic would graduate first, in which case you\nhave a chance to rebase these patches on top of the 'master' that\nalready have the other topic. Your patches would then be consistent\nwith the base commit, as the base would already be cleaned up.\n\nThanks.\n\n"},{"id":"440197","messageId":"20211031171510.1646396-1-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211029184512.1568017-1-eschwartz@archlinux.org","subject":"[PATCH v4 0/3] Add some more options to the pretty-formats","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-31T17:15:07Z","receivedAt":"2021-10-31T17:15:39Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"Renamed enum values. OPT_ -> DESCRIBE_ARG_\nDoc fixups.\n\nEli Schwartz (3):\n pretty.c: rework describe options parsing for better extensibility\n pretty: add tag option to %(describe)\n pretty: add abbrev option to %(describe)\n\n Documentation/pretty-formats.txt | 16 ++++++---\n pretty.c | 58 ++++++++++++++++++++++++++------\n t/t4205-log-pretty-formats.sh | 16 +++++++++\n 3 files changed, 75 insertions(+), 15 deletions(-)\n\nRange-diff against v3:\n1: 55a20468d3 ! 1: be35fee252 pretty.c: rework describe options parsing for better extensibility\n @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,\n -\tconst char *options[] = { \"match\", \"exclude\" };\n +\tstruct {\n +\t\tchar *name;\n -+\t\tenum { OPT_STRING } type;\n ++\t\tenum {\n ++\t\t\tDESCRIBE_ARG_STRING,\n ++\t\t} type;\n +\t} option[] = {\n -+\t\t{ \"exclude\", OPT_STRING },\n -+\t\t{ \"match\", OPT_STRING },\n ++\t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n ++\t\t{ \"match\", DESCRIBE_ARG_STRING },\n +\t};\n \tconst char *arg = start;\n \n @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,\n -\t\t\t\tmatched = options[i];\n +\t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n +\t\t\tswitch (option[i].type) {\n -+\t\t\tcase OPT_STRING:\n ++\t\t\tcase DESCRIBE_ARG_STRING:\n +\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n +\t\t\t\t\t\t\t\t&argval, &arglen)) {\n +\t\t\t\t\tif (!arglen)\n2: c34c8a4f7f ! 2: 5830c69d4d pretty: add tag option to %(describe)\n @@ Documentation/pretty-formats.txt: The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n -+** 'tags[=]': Instead of only considering annotated tags,\n ++** 'tags[=]': Instead of only considering annotated tags,\n + consider lightweight tags as well.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n @@ Documentation/pretty-formats.txt: insert an empty string unless we are traversin\n decoration format if `--decorate` was not already provided on the command\n line.\n \n -+The boolean options accept an optional value `[=]`. The values\n ++The boolean options accept an optional value `[=]`. The values\n +`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n +option is given with no value, it's enabled.\n @@ Documentation/pretty-formats.txt: insert an empty string unless we are traversin\n \n ## pretty.c ##\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)\n - {\n \tstruct {\n \t\tchar *name;\n --\t\tenum { OPT_STRING } type;\n -+\t\tenum { OPT_BOOL, OPT_STRING, } type;\n + \t\tenum {\n ++\t\t\tDESCRIBE_ARG_BOOL,\n + \t\t\tDESCRIBE_ARG_STRING,\n + \t\t} type;\n \t} option[] = {\n -+\t\t{ \"tags\", OPT_BOOL},\n - \t\t{ \"exclude\", OPT_STRING },\n - \t\t{ \"match\", OPT_STRING },\n ++\t\t{ \"tags\", DESCRIBE_ARG_BOOL},\n + \t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n + \t\t{ \"match\", DESCRIBE_ARG_STRING },\n \t};\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tint found = 0;\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n \n \t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n \t\t\tswitch (option[i].type) {\n -+\t\t\tcase OPT_BOOL:\n ++\t\t\tcase DESCRIBE_ARG_BOOL:\n +\t\t\t\tif (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n +\t\t\t\t\tif (optval)\n +\t\t\t\t\t\tstrvec_pushf(args, \"--%s\", option[i].name);\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n +\t\t\t\t\tfound = 1;\n +\t\t\t\t}\n +\t\t\t\tbreak;\n - \t\t\tcase OPT_STRING:\n + \t\t\tcase DESCRIBE_ARG_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\n \n3: b751aaf3c6 ! 3: 032513150d pretty: add abbrev option to %(describe)\n @@ Commit message\n ## Documentation/pretty-formats.txt ##\n @@ Documentation/pretty-formats.txt: The placeholders are:\n +\n - ** 'tags[=]': Instead of only considering annotated tags,\n + ** 'tags[=]': Instead of only considering annotated tags,\n consider lightweight tags as well.\n -+** 'abbrev=': Instead of using the default number of hexadecimal digits\n ++** 'abbrev=': Instead of using the default number of hexadecimal digits\n + (which will vary according to the number of objects in the repository with a\n -+ default of 7) of the abbreviated object name, use digits, or as many digits\n -+ as needed to form a unique object name.\n ++ default of 7) of the abbreviated object name, use digits, or as many\n ++ digits as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n \n ## pretty.c ##\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)\n - {\n - \tstruct {\n \t\tchar *name;\n --\t\tenum { OPT_BOOL, OPT_STRING, } type;\n -+\t\tenum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;\n + \t\tenum {\n + \t\t\tDESCRIBE_ARG_BOOL,\n ++\t\t\tDESCRIBE_ARG_INTEGER,\n + \t\t\tDESCRIBE_ARG_STRING,\n + \t\t} type;\n \t} option[] = {\n - \t\t{ \"tags\", OPT_BOOL},\n -+\t\t{ \"abbrev\", OPT_INTEGER },\n - \t\t{ \"exclude\", OPT_STRING },\n - \t\t{ \"match\", OPT_STRING },\n + \t\t{ \"tags\", DESCRIBE_ARG_BOOL},\n ++\t\t{ \"abbrev\", DESCRIBE_ARG_INTEGER },\n + \t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n + \t\t{ \"match\", DESCRIBE_ARG_STRING },\n \t};\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\t\t\t\tfound = 1;\n \t\t\t\t}\n \t\t\t\tbreak;\n -+\t\t\tcase OPT_INTEGER:\n ++\t\t\tcase DESCRIBE_ARG_INTEGER:\n +\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n +\t\t\t\t\t\t\t\t&argval, &arglen)) {\n +\t\t\t\t\tchar *endptr;\n @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar\n +\t\t\t\t\tfound = 1;\n +\t\t\t\t}\n +\t\t\t\tbreak;\n - \t\t\tcase OPT_STRING:\n + \t\t\tcase DESCRIBE_ARG_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\n \n-- \n2.33.1\n\n"},{"id":"440198","messageId":"20211031171510.1646396-2-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211031171510.1646396-1-eschwartz@archlinux.org","subject":"[PATCH v4 1/3] pretty.c: rework describe options parsing for better extensibility","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-31T17:15:08Z","receivedAt":"2021-10-31T17:15:45Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"It contains option arguments only, not options. We would like to add\noption support here too, but to do that we need to distinguish between\ndifferent types of options.\n\nLay out the groundwork for distinguishing between bools, strings, etc.\nand move the central logic (validating values and pushing new arguments\nto *args) into the successful match, because that will be fairly\nconditional on what type of argument is being parsed.\n\nSigned-off-by: Eli Schwartz \n---\n pretty.c | 31 +++++++++++++++++++++----------\n 1 file changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex be477bd51f..c38acda8cb 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1212,28 +1212,39 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \n static size_t parse_describe_args(const char *start, struct strvec *args)\n {\n-\tconst char *options[] = { \"match\", \"exclude\" };\n+\tstruct {\n+\t\tchar *name;\n+\t\tenum {\n+\t\t\tDESCRIBE_ARG_STRING,\n+\t\t} type;\n+\t} option[] = {\n+\t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n+\t\t{ \"match\", DESCRIBE_ARG_STRING },\n+\t};\n \tconst char *arg = start;\n \n \tfor (;;) {\n-\t\tconst char *matched = NULL;\n+\t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n \t\tint i;\n \n-\t\tfor (i = 0; i < ARRAY_SIZE(options); i++) {\n-\t\t\tif (match_placeholder_arg_value(arg, options[i], &arg,\n-\t\t\t\t\t\t\t&argval, &arglen)) {\n-\t\t\t\tmatched = options[i];\n+\t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n+\t\t\tswitch (option[i].type) {\n+\t\t\tcase DESCRIBE_ARG_STRING:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen)) {\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n-\t\tif (!matched)\n+\t\tif (!found)\n \t\t\tbreak;\n \n-\t\tif (!arglen)\n-\t\t\treturn 0;\n-\t\tstrvec_pushf(args, \"--%s=%.*s\", matched, (int)arglen, argval);\n \t}\n \treturn arg - start;\n }\n-- \n2.33.1\n\n"},{"id":"440199","messageId":"20211031171510.1646396-3-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211031171510.1646396-1-eschwartz@archlinux.org","subject":"[PATCH v4 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-31T17:15:09Z","receivedAt":"2021-10-31T17:15:45Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, only\nsupports annotated tags. However, some people do use lightweight tags\nfor releases, and would like to describe those anyway. The command line\ntool has an option to support this.\n\nTeach the placeholder to support this as well.\n\nSigned-off-by: Eli Schwartz \n---\n\nI use lowercase \"bool\" here not \"boolean-value\" because I don't see\nutility in the word \"value\" here.\n\n Documentation/pretty-formats.txt | 12 +++++++-----\n pretty.c | 12 ++++++++++++\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex ef6bd420ae..1ee47bd4a3 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -220,6 +220,8 @@ The placeholders are:\n \t\t\t inconsistent when tags are added or removed at\n \t\t\t the same time.\n +\n+** 'tags[=]': Instead of only considering annotated tags,\n+ consider lightweight tags as well.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\n@@ -273,11 +275,6 @@ endif::git-rev-list[]\n \t\t\t If any option is provided multiple times the\n \t\t\t last occurrence wins.\n +\n-The boolean options accept an optional value `[=]`. The values\n-`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n-sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n-option is given with no value, it's enabled.\n-+\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by\n decoration format if `--decorate` was not already provided on the command\n line.\n \n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n+\n If you add a `+` (plus sign) after '%' of a placeholder, a line-feed\n is inserted immediately before the expansion if and only if the\n placeholder expands to a non-empty string.\ndiff --git a/pretty.c b/pretty.c\nindex c38acda8cb..403d89725a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1215,9 +1215,11 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \tstruct {\n \t\tchar *name;\n \t\tenum {\n+\t\t\tDESCRIBE_ARG_BOOL,\n \t\t\tDESCRIBE_ARG_STRING,\n \t\t} type;\n \t} option[] = {\n+\t\t{ \"tags\", DESCRIBE_ARG_BOOL},\n \t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n \t\t{ \"match\", DESCRIBE_ARG_STRING },\n \t};\n@@ -1227,10 +1229,20 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tint found = 0;\n \t\tconst char *argval;\n \t\tsize_t arglen = 0;\n+\t\tint optval = 0;\n \t\tint i;\n \n \t\tfor (i = 0; !found && i < ARRAY_SIZE(option); i++) {\n \t\t\tswitch (option[i].type) {\n+\t\t\tcase DESCRIBE_ARG_BOOL:\n+\t\t\t\tif (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {\n+\t\t\t\t\tif (optval)\n+\t\t\t\t\t\tstrvec_pushf(args, \"--%s\", option[i].name);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrvec_pushf(args, \"--no-%s\", option[i].name);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase DESCRIBE_ARG_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5865daa8f8..d4acf8882f 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:tags) vs git describe --tags' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag tagname &&\n+\tgit describe --tags >expect &&\n+\tgit log -1 --format=\"%(describe:tags)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440200","messageId":"20211031171510.1646396-4-eschwartz@archlinux.org","threadId":"56770","inReplyTo":"20211031171510.1646396-1-eschwartz@archlinux.org","subject":"[PATCH v4 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-31T17:15:10Z","receivedAt":"2021-10-31T17:15:45Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"The %(describe) placeholder by default, like `git describe`, uses a\nseven-character abbreviated commit object name. This may not be\nsufficient to fully describe all commits in a given repository,\nresulting in a placeholder replacement changing its length because the\nrepository grew in size. This could cause the output of git-archive to\nchange.\n\nAdd the --abbrev option to `git describe` to the placeholder interface\nin order to provide tools to the user for fine-tuning project defaults\nand ensure reproducible archives.\n\nOne alternative would be to just always specify --abbrev=40 but this may\nbe a bit too biased...\n\nSigned-off-by: Eli Schwartz \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 15 +++++++++++++++\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 1ee47bd4a3..9e943fb74b 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -222,6 +222,10 @@ The placeholders are:\n +\n ** 'tags[=]': Instead of only considering annotated tags,\n consider lightweight tags as well.\n+** 'abbrev=': Instead of using the default number of hexadecimal digits\n+ (which will vary according to the number of objects in the repository with a\n+ default of 7) of the abbreviated object name, use digits, or as many\n+ digits as needed to form a unique object name.\n ** 'match=': Only consider tags matching the given\n `glob(7)` pattern, excluding the \"refs/tags/\" prefix.\n ** 'exclude=': Do not consider tags matching the given\ndiff --git a/pretty.c b/pretty.c\nindex 403d89725a..fa9bfea273 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1216,10 +1216,12 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\tchar *name;\n \t\tenum {\n \t\t\tDESCRIBE_ARG_BOOL,\n+\t\t\tDESCRIBE_ARG_INTEGER,\n \t\t\tDESCRIBE_ARG_STRING,\n \t\t} type;\n \t} option[] = {\n \t\t{ \"tags\", DESCRIBE_ARG_BOOL},\n+\t\t{ \"abbrev\", DESCRIBE_ARG_INTEGER },\n \t\t{ \"exclude\", DESCRIBE_ARG_STRING },\n \t\t{ \"match\", DESCRIBE_ARG_STRING },\n \t};\n@@ -1243,6 +1245,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)\n \t\t\t\t\tfound = 1;\n \t\t\t\t}\n \t\t\t\tbreak;\n+\t\t\tcase DESCRIBE_ARG_INTEGER:\n+\t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n+\t\t\t\t\t\t\t\t&argval, &arglen)) {\n+\t\t\t\t\tchar *endptr;\n+\t\t\t\t\tif (!arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrtol(argval, &endptr, 10);\n+\t\t\t\t\tif (endptr - argval != arglen)\n+\t\t\t\t\t\treturn 0;\n+\t\t\t\t\tstrvec_pushf(args, \"--%s=%.*s\", option[i].name, (int)arglen, argval);\n+\t\t\t\t\tfound = 1;\n+\t\t\t\t}\n+\t\t\t\tbreak;\n \t\t\tcase DESCRIBE_ARG_STRING:\n \t\t\t\tif (match_placeholder_arg_value(arg, option[i].name, &arg,\n \t\t\t\t\t\t\t\t&argval, &arglen)) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex d4acf8882f..35eef4c865 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '\n+\ttest_when_finished \"git tag -d tagname\" &&\n+\tgit tag -a -m tagged tagname &&\n+\tgit describe --abbrev=15 >expect &&\n+\tgit log -1 --format=\"%(describe:abbrev=15)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1\n\n"},{"id":"440201","messageId":"xmqqh7cxdq4k.fsf@gitster.g","threadId":"56770","inReplyTo":"20211031171510.1646396-3-eschwartz@archlinux.org","subject":"Re: [PATCH v4 2/3] pretty: add tag option to %(describe)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-31T18:07:55Z","receivedAt":"2021-10-31T18:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Schwartz writes:\n\n> The %(describe) placeholder by default, like `git describe`, only\n> supports annotated tags. However, some people do use lightweight tags\n> for releases, and would like to describe those anyway. The command line\n> tool has an option to support this.\n>\n> Teach the placeholder to support this as well.\n>\n> Signed-off-by: Eli Schwartz \n> ---\n>\n> I use lowercase \"bool\" here not \"boolean-value\" because I don't see\n> utility in the word \"value\" here.\n\nSuch a comment is much more useful if it is sent as a review to the\npatch that touches the same area as your patch does, namely,\n\nhttps://lore.kernel.org/git/984b6d687a2e779c775de6ea80536afe6ecc0aaf.1635438124.git.gitgitgadget@gmail.com/\n\nnot here.\n\nThanks.\n\n[jc: added a few folks involved in the other patch to the addressee\nlists]\n"},{"id":"440204","messageId":"3939e806-8bb8-b6ac-1e3b-870f31746603@archlinux.org","threadId":"56770","inReplyTo":"xmqqh7cxdq4k.fsf@gitster.g","subject":"Re: [PATCH v4 2/3] pretty: add tag option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-10-31T18:58:48Z","receivedAt":"2021-10-31T18:59:18Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 10/31/21 2:07 PM, Junio C Hamano wrote:\n> Eli Schwartz writes:\n> \n>> The %(describe) placeholder by default, like `git describe`, only\n>> supports annotated tags. However, some people do use lightweight tags\n>> for releases, and would like to describe those anyway. The command line\n>> tool has an option to support this.\n>>\n>> Teach the placeholder to support this as well.\n>>\n>> Signed-off-by: Eli Schwartz \n>> ---\n>>\n>> I use lowercase \"bool\" here not \"boolean-value\" because I don't see\n>> utility in the word \"value\" here.\n> \n> Such a comment is much more useful if it is sent as a review to the\n> patch that touches the same area as your patch does, namely,\n> \n> https://lore.kernel.org/git/984b6d687a2e779c775de6ea80536afe6ecc0aaf.1635438124.git.gitgitgadget@gmail.com/\n> \n> not here.\n\n\nIndeed, done. (I had to unexpectedly step away for a bit after sending\nmy updated series.)\n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"},{"id":"440407","messageId":"nycvar.QRO.7.76.6.2111040018240.56@tvgsbejvaqbjf.bet","threadId":"56770","inReplyTo":"43fe6d5c-bdb2-585c-c601-1da7a1b3ff8b@archlinux.org","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-03T23:20:34Z","receivedAt":"2021-11-03T23:20:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eli,\n\nOn Tue, 26 Oct 2021, Eli Schwartz wrote:\n\n> On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:\n> > Other than the question pointed out by Eric,\n> >\n> > with DEVELOPER=1, -Werror=declaration-after-statement\n> > We'll need this change squashed in:\n>\n>\n> Thanks for the advice. In v1 of this patchset I attempted to do a\n> developer build but failed due to preexisting errors:\n>\n>\n> CC run-command.o\n> run-command.c: In function ‘async_die_is_recursing’:\n> run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a\n> region of size 0 [-Werror=stringop-overread]\n> 1102 | pthread_setspecific(async_die_counter, (void *)1);\n> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> In file included from /usr/include/openssl/crypto.h:415,\n> from /usr/include/openssl/comp.h:16,\n> from /usr/include/openssl/ssl.h:17,\n> from git-compat-util.h:309,\n> from cache.h:4,\n> from run-command.c:1:\n> /usr/include/pthread.h:1308:12: note: in a call to function\n> ‘pthread_setspecific’ declared with attribute ‘access (none, 2)’\n> 1308 | extern int pthread_setspecific (pthread_key_t __key,\n> | ^~~~~~~~~~~~~~~~~~~\n> cc1: all warnings being treated as errors\n>\n>\n>\n> My system has a custom compiled glibc from git roughly around the 2.34\n> release (a similar environment could be obtained by using Fedora rawhide\n> I guess), and this commit looks mighty suspicious:\n> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0\n>\n> For this reason, I did not bother to try testing v2 under a developer\n> build, leading to my overlooking this issue. ;)\n\nIt seems that this issue now hit an official version. As I explained in\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,\nmy colleague Victoria Dye will send a fix for this later.\n\nStay tuned,\nJohannes\n"},{"id":"440435","messageId":"nycvar.QRO.7.76.6.2111041028170.56@tvgsbejvaqbjf.bet","threadId":"56770","inReplyTo":"nycvar.QRO.7.76.6.2111040018240.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-04T09:29:04Z","receivedAt":"2021-11-04T09:29:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eli,\n\nOn Thu, 4 Nov 2021, Johannes Schindelin wrote:\n\n> On Tue, 26 Oct 2021, Eli Schwartz wrote:\n>\n> > On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:\n> > > Other than the question pointed out by Eric,\n> > >\n> > > with DEVELOPER=1, -Werror=declaration-after-statement\n> > > We'll need this change squashed in:\n> >\n> >\n> > Thanks for the advice. In v1 of this patchset I attempted to do a\n> > developer build but failed due to preexisting errors:\n> >\n> >\n> > CC run-command.o\n> > run-command.c: In function ‘async_die_is_recursing’:\n> > run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a\n> > region of size 0 [-Werror=stringop-overread]\n> > 1102 | pthread_setspecific(async_die_counter, (void *)1);\n> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > In file included from /usr/include/openssl/crypto.h:415,\n> > from /usr/include/openssl/comp.h:16,\n> > from /usr/include/openssl/ssl.h:17,\n> > from git-compat-util.h:309,\n> > from cache.h:4,\n> > from run-command.c:1:\n> > /usr/include/pthread.h:1308:12: note: in a call to function\n> > ‘pthread_setspecific’ declared with attribute ‘access (none, 2)’\n> > 1308 | extern int pthread_setspecific (pthread_key_t __key,\n> > | ^~~~~~~~~~~~~~~~~~~\n> > cc1: all warnings being treated as errors\n> >\n> >\n> >\n> > My system has a custom compiled glibc from git roughly around the 2.34\n> > release (a similar environment could be obtained by using Fedora rawhide\n> > I guess), and this commit looks mighty suspicious:\n> > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0\n> >\n> > For this reason, I did not bother to try testing v2 under a developer\n> > build, leading to my overlooking this issue. ;)\n>\n> It seems that this issue now hit an official version. As I explained in\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,\n> my colleague Victoria Dye will send a fix for this later.\n\nFYI here is the patch:\nhttps://lore.kernel.org/git/pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com/\n\nCiao,\nJohannes\n"},{"id":"440636","messageId":"13499fc1-1ac0-0a3a-b328-3aab5c37602b@archlinux.org","threadId":"56770","inReplyTo":"nycvar.QRO.7.76.6.2111040018240.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] pretty: add abbrev option to %(describe)","fromName":"Eli Schwartz","fromEmail":"eschwartz@archlinux.org","sentAt":"2021-11-07T12:39:23Z","receivedAt":"2021-11-07T12:40:19Z","isPatch":true,"sender":{"key":"eschwartz@archlinux.org","avatar":"https://avatars.githubusercontent.com/u/6551424?v=4"},"body":"On 11/3/21 7:20 PM, Johannes Schindelin wrote:\n> Hi Eli,\n> \n> On Tue, 26 Oct 2021, Eli Schwartz wrote:\n> \n>> My system has a custom compiled glibc from git roughly around the 2.34\n>> release (a similar environment could be obtained by using Fedora rawhide\n>> I guess), and this commit looks mighty suspicious:\n>> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0\n>>\n>> For this reason, I did not bother to try testing v2 under a developer\n>> build, leading to my overlooking this issue. ;)\n> \n> It seems that this issue now hit an official version. As I explained in\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,\n> my colleague Victoria Dye will send a fix for this later.\n> \n> Stay tuned,\n\n\nFWIW this was present in the official version of glibc released in\nAugust... the problem is finding an official version of a distro that\nships it. :D\n\nThanks for the heads up.\n\n\n-- \nEli Schwartz\nArch Linux Bug Wrangler and Trusted User\n"}]}