[PATCH 0/3] Add some more options to the pretty-formats

While discussing adding git archive support to a software versioning
tool, the issue came up that apparently many people (maybe in the python
community specifically?) use lightweight tags for releases.

While it would be nice if the current describe functionality for
export-subst was sufficient to cover the average reasonable use, this is
apparently not the case; at least --tags support will most likely need
to be added. The alternative is documenting a workflow change and having
the versioning tool raise some kind of error if you use the wrong kind
of tag, which is not an exciting requirement for the project maintainer.

In my initial proposal of the %(describe) feature I gave an example
using --tags, but it never ended up in the initial implementation:

https://public-inbox.org/git/7418f1d8-78c2-61a7-4f03-62360b986a41@archlinux.org/

So I figured I'd take a stab at it myself. While I was at it, I looked
at the options available to git describe and came up with a use case for
adding --abbrev support too.

Eli Schwartz (3):
  pretty.c: rename describe options variable to more descriptive name
  pretty: add tag option to %(describe)
  pretty: add abbrev option to %(describe)

 Documentation/pretty-formats.txt | 15 ++++++++++-----
 pretty.c                         | 31 +++++++++++++++++++++++--------
 t/t4205-log-pretty-formats.sh    | 16 ++++++++++++++++
 3 files changed, 49 insertions(+), 13 deletions(-)

-- 
2.33.1

It contains option arguments, not options. We would like to add option
support here too.

Signed-off-by: Eli Schwartz <redacted>
---
 pretty.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index 73b5ead509..9db2c65538 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
-	const char *options[] = { "match", "exclude" };
+	const char *option_arguments[] = { "match", "exclude" };
 	const char *arg = start;
 
 	for (;;) {

@@ -1225,10 +1225,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 		size_t arglen = 0;
 		int i;
 
-		for (i = 0; i < ARRAY_SIZE(options); i++) {
-			if (match_placeholder_arg_value(arg, options[i], &arg,
+		for (i = 0; i < ARRAY_SIZE(option_arguments); i++) {
+			if (match_placeholder_arg_value(arg, option_arguments[i], &arg,
 							&argval, &arglen)) {
-				matched = options[i];
+				matched = option_arguments[i];
 				break;
 			}
 		}

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, uses a
seven-character abbreviated commit hash. This may not be sufficient to
fully describe all git repos, resulting in a placeholder replacement
changing its length because the repository grew in size. This could
cause the output of git-archive to change.

Add the --abbrev option to `git describe` to the placeholder interface
in order to provide tools to the user for fine-tuning project defaults
and ensure reproducible archives.

One alternative would be to just always specify --abbrev=40 but this may
be a bit too biased...

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt | 4 ++++
 pretty.c                         | 2 +-
 t/t4205-log-pretty-formats.sh    | 8 ++++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 14107ac191..317c1382b5 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -221,6 +221,10 @@ The placeholders are:
 			  the same time.
 +
 ** 'tags[=<BOOL>]': Also consider lightweight tags.
+** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
+   (which will vary according to the number of objects in the repository with a
+   default of 7) of the abbreviated object name, use <n> digits, or as many digits
+   as needed to form a unique object name.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

diff --git a/pretty.c b/pretty.c
index 3a41bedf1a..a092457274 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1217,7 +1217,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
 	const char *options[] = { "tags" };
-	const char *option_arguments[] = { "match", "exclude" };
+	const char *option_arguments[] = { "match", "exclude", "abbrev" };
 	const char *arg = start;
 
 	for (;;) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d4acf8882f..35eef4c865 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
+	test_when_finished "git tag -d tagname" &&
+	git tag -a -m tagged tagname &&
+	git describe --abbrev=15 >expect &&
+	git log -1 --format="%(describe:abbrev=15)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, only
supports annotated tags. However, some people do use lightweight tags
for releases, and would like to describe those anyway. The command line
tool has an option to support this.

Teach the placeholder to support this as well.

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt | 11 ++++++-----
 pretty.c                         | 23 +++++++++++++++++++----
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index ef6bd420ae..14107ac191 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -220,6 +220,7 @@ The placeholders are:
 			  inconsistent when tags are added or removed at
 			  the same time.
 +
+** 'tags[=<BOOL>]': Also consider lightweight tags.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

@@ -273,11 +274,6 @@ endif::git-rev-list[]
 			  If any option is provided multiple times the
 			  last occurrence wins.
 +
-The boolean options accept an optional value `[=<BOOL>]`. The values
-`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
-sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
-option is given with no value, it's enabled.
-+
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are

@@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
 decoration format if `--decorate` was not already provided on the command
 line.
 
+The boolean options accept an optional value `[=<BOOL>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
+
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
 placeholder expands to a non-empty string.

diff --git a/pretty.c b/pretty.c
index 9db2c65538..3a41bedf1a 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
+	const char *options[] = { "tags" };
 	const char *option_arguments[] = { "match", "exclude" };
 	const char *arg = start;
 
 	for (;;) {
 		const char *matched = NULL;
-		const char *argval;
+		const char *argval = NULL;
 		size_t arglen = 0;
+		int optval = 0;
 		int i;
 
 		for (i = 0; i < ARRAY_SIZE(option_arguments); i++) {
 			if (match_placeholder_arg_value(arg, option_arguments[i], &arg,
 							&argval, &arglen)) {
 				matched = option_arguments[i];
+				if (!arglen)
+					return 0;
 				break;
 			}
 		}
+		if (!matched)
+			for (i = 0; i < ARRAY_SIZE(options); i++) {
+				if (match_placeholder_bool_arg(arg, options[i], &arg,
+								&optval)) {
+					matched = options[i];
+					break;
+				}
+			}
 		if (!matched)
 			break;
 
-		if (!arglen)
-			return 0;
-		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
+
+		if (argval) {
+			strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
+		} else if (optval) {
+			strvec_pushf(args, "--%s", matched);
+		}
 	}
 	return arg - start;
 }

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8..d4acf8882f 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:tags) vs git describe --tags' '
+	test_when_finished "git tag -d tagname" &&
+	git tag tagname &&
+	git describe --tags >expect &&
+	git log -1 --format="%(describe:tags)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

It contains option arguments only, not options. We would like to add
option support here too, but to do that we need to distinguish between
different types of options.

Lay out the groundwork for distinguishing between bools, strings, etc.
and move the central logic (validating values and pushing new arguments
to *args) into the successful match, because that will be fairly
conditional on what type of argument is being parsed.

Signed-off-by: Eli Schwartz <redacted>
---
 pretty.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/pretty.c b/pretty.c
index 73b5ead509..f8b254d2ff 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
-	const char *options[] = { "match", "exclude" };
+	struct {
+		char *name;
+		enum { OPT_STRING } type;
+	}  option[] = {
+		{ "exclude", OPT_STRING },
+		{ "match", OPT_STRING },
+	};
 	const char *arg = start;
 
 	for (;;) {
-		const char *matched = NULL;
+		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
 		int i;
 
-		for (i = 0; i < ARRAY_SIZE(options); i++) {
-			if (match_placeholder_arg_value(arg, options[i], &arg,
-							&argval, &arglen)) {
-				matched = options[i];
+		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
+			switch(option[i].type) {
+			case OPT_STRING:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen) && arglen) {
+					if (!arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
 				break;
 			}
 		}
-		if (!matched)
+		if (!found)
 			break;
 
-		if (!arglen)
-			return 0;
-		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
 	}
 	return arg - start;
 }

-- 
2.33.1

Here is the reworked implementation and all-new replacement first patch,
as suggested by review comments.

Minor tweaks to documentation in the second patch, otherwise
documentation and test cases are the same.

Eli Schwartz (3):
  pretty.c: rework describe options parsing for better extensibility
  pretty: add tag option to %(describe)
  pretty: add abbrev option to %(describe)

 Documentation/pretty-formats.txt | 16 +++++++---
 pretty.c                         | 55 ++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh    | 16 ++++++++++
 3 files changed, 72 insertions(+), 15 deletions(-)

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, only
supports annotated tags. However, some people do use lightweight tags
for releases, and would like to describe those anyway. The command line
tool has an option to support this.

Teach the placeholder to support this as well.

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt | 12 +++++++-----
 pretty.c                         | 14 +++++++++++++-
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index ef6bd420ae..86ed801aad 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -220,6 +220,8 @@ The placeholders are:
 			  inconsistent when tags are added or removed at
 			  the same time.
 +
+** 'tags[=<BOOL>]': Instead of only considering annotated tags,
+   consider lightweight tags as well.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

@@ -273,11 +275,6 @@ endif::git-rev-list[]
 			  If any option is provided multiple times the
 			  last occurrence wins.
 +
-The boolean options accept an optional value `[=<BOOL>]`. The values
-`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
-sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
-option is given with no value, it's enabled.
-+
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are

@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
 decoration format if `--decorate` was not already provided on the command
 line.
 
+The boolean options accept an optional value `[=<BOOL>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
+
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
 placeholder expands to a non-empty string.

diff --git a/pretty.c b/pretty.c
index f8b254d2ff..16b5366fed 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1218,8 +1218,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 {
 	struct {
 		char *name;
-		enum { OPT_STRING } type;
+		enum { OPT_BOOL, OPT_STRING, } type;
 	}  option[] = {
+		{ "tags", OPT_BOOL},
 		{ "exclude", OPT_STRING },
 		{ "match", OPT_STRING },
 	};

@@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
+		int optval = 0;
 		int i;
 
 		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
 			switch(option[i].type) {
+			case OPT_BOOL:
+				if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
+					if (optval) {
+						strvec_pushf(args, "--%s", option[i].name);
+					} else {
+						strvec_pushf(args, "--no-%s", option[i].name);
+					}
+					found = 1;
+				}
+				break;
 			case OPT_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen) && arglen) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8..d4acf8882f 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:tags) vs git describe --tags' '
+	test_when_finished "git tag -d tagname" &&
+	git tag tagname &&
+	git describe --tags >expect &&
+	git log -1 --format="%(describe:tags)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, uses a
seven-character abbreviated commit object name. This may not be
sufficient to fully describe all commits in a given repository,
resulting in a placeholder replacement changing its length because the
repository grew in size.  This could cause the output of git-archive to
change.

Add the --abbrev option to `git describe` to the placeholder interface
in order to provide tools to the user for fine-tuning project defaults
and ensure reproducible archives.

One alternative would be to just always specify --abbrev=40 but this may
be a bit too biased...

Signed-off-by: Eli Schwartz <redacted>
---

Notes:
    With regard to validating that an integer is passed, I attempt to parse the
    result using the same mechanism git-describe itself does in the abbrev
    callback, just with slightly different validation of what we have at the end...
    because of course here argval is the entire rest of the format string,
    including the ")".
    
    While testing that this actually does what it's supposed to do, I noticed that
    it doesn't validate junk like leading whitespace or plus signs... this is a
    problem for `git describe --abbrev='    +15'` too so I guess it's not my
    problem to fix...

 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         | 16 +++++++++++++++-
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86ed801aad..57fd84f579 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -222,6 +222,10 @@ The placeholders are:
 +
 ** 'tags[=<BOOL>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
+** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
+   (which will vary according to the number of objects in the repository with a
+   default of 7) of the abbreviated object name, use <n> digits, or as many digits
+   as needed to form a unique object name.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

diff --git a/pretty.c b/pretty.c
index 16b5366fed..44bfc49b38 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 {
 	struct {
 		char *name;
-		enum { OPT_BOOL, OPT_STRING, } type;
+		enum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;
 	}  option[] = {
 		{ "tags", OPT_BOOL},
+		{ "abbrev", OPT_INTEGER },
 		{ "exclude", OPT_STRING },
 		{ "match", OPT_STRING },
 	};

@@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 					found = 1;
 				}
 				break;
+			case OPT_INTEGER:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen) && arglen) {
+					if (!arglen)
+						return 0;
+					char* endptr;
+					strtol(argval, &endptr, 10);
+					if (endptr - argval != arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
+				break;
 			case OPT_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen) && arglen) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d4acf8882f..35eef4c865 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
+	test_when_finished "git tag -d tagname" &&
+	git tag -a -m tagged tagname &&
+	git describe --abbrev=15 >expect &&
+	git log -1 --format="%(describe:abbrev=15)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz [off-list ref] wrote:

I may be missing something obvious, but how will it be possible for:

    if (!arglen)
        return 0;

to trigger if the `if` immediately above it:

    if (... && arglen) {

 has already asserted that `arglen` is not 0?

+                                       strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+                                       found = 1;
+                               }
                                break;
                        }
                }
+               if (!found)
                        break;

The use of `found` to break out of a loop from within a `switch` seems
a bit clunky. An alternative would be to `goto` a label...

        }
        return arg - start;

... which could be introduced just before the `return`. Of course,
this is highly subjective, so not necessarily worth changing.

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz [off-list ref] wrote:

Style nit: add space after `if`

+                                       if (optval) {
+                                               strvec_pushf(args, "--%s", option[i].name);
+                                       } else {
+                                               strvec_pushf(args, "--no-%s", option[i].name);
+                                       }

We would normally omit the braces for this simple `if`:

    if (optval)
        strvec_pushf(...);
    else
        strvec_pushf(...);

... or maybe even use the ternary operator:

    strvec_pushf(args, "--%s%s", optval ? "" : "no-", option[i].name);

but it's highly subjective whether or not that's more readable.

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz [off-list ref] wrote:

Inconsistent mix of `<N>` and `<n>`.

Same question I asked while reviewing the other patch regarding
checking `arglen` in both conditionals: `if (... && arglen)` vs. `if
(!arglen)`

On 2021-10-25 21:34:52-0400, Eli Schwartz [off-list ref] wrote:

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

------- 8< -----

diff --git a/pretty.c b/pretty.c
index 289b5456c8..85d4ab008b 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1249,9 +1249,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 			case OPT_INTEGER:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen) && arglen) {
+					char* endptr;
 					if (!arglen)
 						return 0;
-					char* endptr;
 					strtol(argval, &endptr, 10);
 					if (endptr - argval != arglen)
 						return 0;
------- >8 -----

-- 
Danh

On Tue, Oct 26, 2021 at 8:07 AM Đoàn Trần Công Danh
[off-list ref] wrote:

On 2021-10-25 21:34:52-0400, Eli Schwartz [off-list ref] wrote:

quoted

+                                     if (!arglen)
+                                             return 0;
+                                     char* endptr;

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

+                                       char* endptr;
                                        if (!arglen)
                                                return 0;
-                                       char* endptr;

This highlights a style nit, as well; should be:

    char *endptr;

On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

Thanks for the advice. In v1 of this patchset I attempted to do a
developer build but failed due to preexisting errors:


    CC run-command.o
run-command.c: In function ‘async_die_is_recursing’:
run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a
region of size 0 [-Werror=stringop-overread]
 1102 |         pthread_setspecific(async_die_counter, (void *)1);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/openssl/crypto.h:415,
                 from /usr/include/openssl/comp.h:16,
                 from /usr/include/openssl/ssl.h:17,
                 from git-compat-util.h:309,
                 from cache.h:4,
                 from run-command.c:1:
/usr/include/pthread.h:1308:12: note: in a call to function
‘pthread_setspecific’ declared with attribute ‘access (none, 2)’
 1308 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors



My system has a custom compiled glibc from git roughly around the 2.34
release (a similar environment could be obtained by using Fedora rawhide
I guess), and this commit looks mighty suspicious:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0

For this reason, I did not bother to try testing v2 under a developer
build, leading to my overlooking this issue. ;)

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

On 10/26/21 1:18 AM, Eric Sunshine wrote:

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz [off-list ref] wrote:

quoted

It contains option arguments only, not options. We would like to add
option support here too, but to do that we need to distinguish between
different types of options.

Lay out the groundwork for distinguishing between bools, strings, etc.
and move the central logic (validating values and pushing new arguments
to *args) into the successful match, because that will be fairly
conditional on what type of argument is being parsed.

Signed-off-by: Eli Schwartz <redacted>
---

diff --git a/pretty.c b/pretty.c

@@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
+       struct {
+               char *name;
+               enum { OPT_STRING } type;
+       }  option[] = {
+               { "exclude", OPT_STRING },
+               { "match", OPT_STRING },
+       };
        const char *arg = start;

        for (;;) {
+               int found = 0;
                const char *argval;
                size_t arglen = 0;
                int i;

+               for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
+                       switch(option[i].type) {
+                       case OPT_STRING:
+                               if (match_placeholder_arg_value(arg, option[i].name, &arg,
+                                                               &argval, &arglen) && arglen) {
+                                       if (!arglen)
+                                               return 0;

I may be missing something obvious, but how will it be possible for:

    if (!arglen)
        return 0;

to trigger if the `if` immediately above it:

    if (... && arglen) {

 has already asserted that `arglen` is not 0?

I don't think you are missing anything here, I simply forgot that
halfway through I added a second check to the if, and later moved the
code from down below.

I think returning 0 is correct here, to avoid pointlessly checking the
rest of option[]. So I'll (re-)remove the first check.

quoted

+                                       strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+                                       found = 1;
+                               }
                                break;
                        }
                }
+               if (!found)
                        break;

The use of `found` to break out of a loop from within a `switch` seems
a bit clunky. An alternative would be to `goto` a label...

quoted

        }
        return arg - start;

... which could be introduced just before the `return`. Of course,
this is highly subjective, so not necessarily worth changing.

Keeping in mind that this for (;;) { .... break; } was there before me
:D I just switched the name/type of the variable it checks...

IMO changing to goto is not my business to change (at least not in this
patch), and given the "common wisdom" is "goto is evil" I'm not strongly
inclined to get into the business of rewriting someone else's code for
that. It's too subjective for my taste.

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

On 10/26/21 1:25 AM, Eric Sunshine wrote:

On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz [off-list ref] wrote:

quoted

The %(describe) placeholder by default, like `git describe`, only
supports annotated tags. However, some people do use lightweight tags
for releases, and would like to describe those anyway. The command line
tool has an option to support this.

Teach the placeholder to support this as well.

Signed-off-by: Eli Schwartz <redacted>
---

diff --git a/pretty.c b/pretty.c

@@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
                for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
                        switch(option[i].type) {
+                       case OPT_BOOL:
+                               if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {

Style nit: add space after `if`

Oops, I am not sure how this happened. It's wrong in the switch too.

quoted

+                                       if (optval) {
+                                               strvec_pushf(args, "--%s", option[i].name);
+                                       } else {
+                                               strvec_pushf(args, "--no-%s", option[i].name);
+                                       }

We would normally omit the braces for this simple `if`:

    if (optval)
        strvec_pushf(...);
    else
        strvec_pushf(...);

... or maybe even use the ternary operator:

    strvec_pushf(args, "--%s%s", optval ? "" : "no-", option[i].name);

but it's highly subjective whether or not that's more readable.

Although the braces feel more natural to me for clarity purposes, it's a
good point that the git coding style says to omit them for single
statements, and I should have followed that here.

The ternary doesn't feel readable to me, however.

...

Thanks for the style review!

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

On Wed, Oct 27, 2021 at 12:23 AM Eli Schwartz [off-list ref] wrote:

On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:

quoted

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

Thanks for the advice. In v1 of this patchset I attempted to do a
developer build but failed due to preexisting errors:

you can always avoid breaking your build by using:

  DEVOPTS=no-error

Carlo

This revision only contains style nits in response to review comments.
See below.

Eli Schwartz (3):
  pretty.c: rework describe options parsing for better extensibility
  pretty: add tag option to %(describe)
  pretty: add abbrev option to %(describe)

 Documentation/pretty-formats.txt | 16 +++++++---
 pretty.c                         | 54 ++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh    | 16 ++++++++++
 3 files changed, 71 insertions(+), 15 deletions(-)

Range-diff against v2:
1:  1cf0d82b91 ! 1:  55a20468d3 pretty.c: rework describe options parsing for better extensibility
    @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,
     -							&argval, &arglen)) {
     -				matched = options[i];
     +		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
    -+			switch(option[i].type) {
    ++			switch (option[i].type) {
     +			case OPT_STRING:
     +				if (match_placeholder_arg_value(arg, option[i].name, &arg,
    -+								&argval, &arglen) && arglen) {
    ++								&argval, &arglen)) {
     +					if (!arglen)
     +						return 0;
     +					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
2:  cb6af9bc14 ! 2:  c34c8a4f7f pretty: add tag option to %(describe)
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
      		int i;
      
      		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
    - 			switch(option[i].type) {
    + 			switch (option[i].type) {
     +			case OPT_BOOL:
    -+				if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
    -+					if (optval) {
    ++				if (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
    ++					if (optval)
     +						strvec_pushf(args, "--%s", option[i].name);
    -+					} else {
    ++					else
     +						strvec_pushf(args, "--no-%s", option[i].name);
    -+					}
     +					found = 1;
     +				}
     +				break;
      			case OPT_STRING:
      				if (match_placeholder_arg_value(arg, option[i].name, &arg,
    - 								&argval, &arglen) && arglen) {
    + 								&argval, &arglen)) {
     
      ## t/t4205-log-pretty-formats.sh ##
     @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
3:  08ade18b35 ! 3:  b751aaf3c6 pretty: add abbrev option to %(describe)
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
      				break;
     +			case OPT_INTEGER:
     +				if (match_placeholder_arg_value(arg, option[i].name, &arg,
    -+								&argval, &arglen) && arglen) {
    ++								&argval, &arglen)) {
    ++					char *endptr;
     +					if (!arglen)
     +						return 0;
    -+					char* endptr;
     +					strtol(argval, &endptr, 10);
     +					if (endptr - argval != arglen)
     +						return 0;
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
     +				break;
      			case OPT_STRING:
      				if (match_placeholder_arg_value(arg, option[i].name, &arg,
    - 								&argval, &arglen) && arglen) {
    + 								&argval, &arglen)) {
     
      ## t/t4205-log-pretty-formats.sh ##
     @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(describe:tags) vs git describe --tags' '
-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, only
supports annotated tags. However, some people do use lightweight tags
for releases, and would like to describe those anyway. The command line
tool has an option to support this.

Teach the placeholder to support this as well.

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt | 12 +++++++-----
 pretty.c                         | 13 ++++++++++++-
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index ef6bd420ae..86ed801aad 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -220,6 +220,8 @@ The placeholders are:
 			  inconsistent when tags are added or removed at
 			  the same time.
 +
+** 'tags[=<BOOL>]': Instead of only considering annotated tags,
+   consider lightweight tags as well.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

@@ -273,11 +275,6 @@ endif::git-rev-list[]
 			  If any option is provided multiple times the
 			  last occurrence wins.
 +
-The boolean options accept an optional value `[=<BOOL>]`. The values
-`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
-sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
-option is given with no value, it's enabled.
-+
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are

@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
 decoration format if `--decorate` was not already provided on the command
 line.
 
+The boolean options accept an optional value `[=<BOOL>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
+
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
 placeholder expands to a non-empty string.

diff --git a/pretty.c b/pretty.c
index 2ec023a0d0..a105ef2a15 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1218,8 +1218,9 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 {
 	struct {
 		char *name;
-		enum { OPT_STRING } type;
+		enum { OPT_BOOL, OPT_STRING, } type;
 	}  option[] = {
+		{ "tags", OPT_BOOL},
 		{ "exclude", OPT_STRING },
 		{ "match", OPT_STRING },
 	};

@@ -1229,10 +1230,20 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
+		int optval = 0;
 		int i;
 
 		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
 			switch (option[i].type) {
+			case OPT_BOOL:
+				if (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
+					if (optval)
+						strvec_pushf(args, "--%s", option[i].name);
+					else
+						strvec_pushf(args, "--no-%s", option[i].name);
+					found = 1;
+				}
+				break;
 			case OPT_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen)) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8..d4acf8882f 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:tags) vs git describe --tags' '
+	test_when_finished "git tag -d tagname" &&
+	git tag tagname &&
+	git describe --tags >expect &&
+	git log -1 --format="%(describe:tags)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, uses a
seven-character abbreviated commit object name. This may not be
sufficient to fully describe all commits in a given repository,
resulting in a placeholder replacement changing its length because the
repository grew in size.  This could cause the output of git-archive to
change.

Add the --abbrev option to `git describe` to the placeholder interface
in order to provide tools to the user for fine-tuning project defaults
and ensure reproducible archives.

One alternative would be to just always specify --abbrev=40 but this may
be a bit too biased...

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         | 16 +++++++++++++++-
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 86ed801aad..57fd84f579 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -222,6 +222,10 @@ The placeholders are:
 +
 ** 'tags[=<BOOL>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
+** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
+   (which will vary according to the number of objects in the repository with a
+   default of 7) of the abbreviated object name, use <n> digits, or as many digits
+   as needed to form a unique object name.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

diff --git a/pretty.c b/pretty.c
index a105ef2a15..5662cb2943 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 {
 	struct {
 		char *name;
-		enum { OPT_BOOL, OPT_STRING, } type;
+		enum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;
 	}  option[] = {
 		{ "tags", OPT_BOOL},
+		{ "abbrev", OPT_INTEGER },
 		{ "exclude", OPT_STRING },
 		{ "match", OPT_STRING },
 	};

@@ -1244,6 +1245,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 					found = 1;
 				}
 				break;
+			case OPT_INTEGER:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen)) {
+					char *endptr;
+					if (!arglen)
+						return 0;
+					strtol(argval, &endptr, 10);
+					if (endptr - argval != arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
+				break;
 			case OPT_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen)) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d4acf8882f..35eef4c865 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
+	test_when_finished "git tag -d tagname" &&
+	git tag -a -m tagged tagname &&
+	git describe --abbrev=15 >expect &&
+	git log -1 --format="%(describe:abbrev=15)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

It contains option arguments only, not options. We would like to add
option support here too, but to do that we need to distinguish between
different types of options.

Lay out the groundwork for distinguishing between bools, strings, etc.
and move the central logic (validating values and pushing new arguments
to *args) into the successful match, because that will be fairly
conditional on what type of argument is being parsed.

Signed-off-by: Eli Schwartz <redacted>
---
 pretty.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/pretty.c b/pretty.c
index fe95107ae5..2ec023a0d0 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
-	const char *options[] = { "match", "exclude" };
+	struct {
+		char *name;
+		enum { OPT_STRING } type;
+	}  option[] = {
+		{ "exclude", OPT_STRING },
+		{ "match", OPT_STRING },
+	};
 	const char *arg = start;
 
 	for (;;) {
-		const char *matched = NULL;
+		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
 		int i;
 
-		for (i = 0; i < ARRAY_SIZE(options); i++) {
-			if (match_placeholder_arg_value(arg, options[i], &arg,
-							&argval, &arglen)) {
-				matched = options[i];
+		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
+			switch (option[i].type) {
+			case OPT_STRING:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen)) {
+					if (!arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
 				break;
 			}
 		}
-		if (!matched)
+		if (!found)
 			break;
 
-		if (!arglen)
-			return 0;
-		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
 	}
 	return arg - start;
 }

-- 
2.33.1

On Fri, Oct 29, 2021 at 2:45 PM Eli Schwartz [off-list ref] wrote:

There's still an inconsistent mix of `<N>` and `<n>` here (mentioned
in my earlier review). Is that intentional or just a simple oversight?

On 10/29/21 2:51 PM, Eric Sunshine wrote:

On Fri, Oct 29, 2021 at 2:45 PM Eli Schwartz [off-list ref] wrote:

quoted

The %(describe) placeholder by default, like `git describe`, uses a
seven-character abbreviated commit object name. This may not be
sufficient to fully describe all commits in a given repository,
resulting in a placeholder replacement changing its length because the
repository grew in size.  This could cause the output of git-archive to
change.

Add the --abbrev option to `git describe` to the placeholder interface
in order to provide tools to the user for fine-tuning project defaults
and ensure reproducible archives.
[...]
Signed-off-by: Eli Schwartz <redacted>
---

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt

@@ -222,6 +222,10 @@ The placeholders are:
+** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
+   (which will vary according to the number of objects in the repository with a
+   default of 7) of the abbreviated object name, use <n> digits, or as many digits
+   as needed to form a unique object name.

There's still an inconsistent mix of `<N>` and `<n>` here (mentioned
in my earlier review). Is that intentional or just a simple oversight?

Ah, sorry... I overlooked that. It was originally copied from the
git-describe man page which uses lowercase and I overlooked that part of
your review.

It should be consistently uppercase here for consistency with
pretty-formats.


-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

Renamed enum values. OPT_ -> DESCRIBE_ARG_
Doc fixups.

Eli Schwartz (3):
  pretty.c: rework describe options parsing for better extensibility
  pretty: add tag option to %(describe)
  pretty: add abbrev option to %(describe)

 Documentation/pretty-formats.txt | 16 ++++++---
 pretty.c                         | 58 ++++++++++++++++++++++++++------
 t/t4205-log-pretty-formats.sh    | 16 +++++++++
 3 files changed, 75 insertions(+), 15 deletions(-)

Range-diff against v3:
1:  55a20468d3 ! 1:  be35fee252 pretty.c: rework describe options parsing for better extensibility
    @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,
     -	const char *options[] = { "match", "exclude" };
     +	struct {
     +		char *name;
    -+		enum { OPT_STRING } type;
    ++		enum {
    ++			DESCRIBE_ARG_STRING,
    ++		} type;
     +	}  option[] = {
    -+		{ "exclude", OPT_STRING },
    -+		{ "match", OPT_STRING },
    ++		{ "exclude", DESCRIBE_ARG_STRING },
    ++		{ "match", DESCRIBE_ARG_STRING },
     +	};
      	const char *arg = start;
      
    @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,
     -				matched = options[i];
     +		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
     +			switch (option[i].type) {
    -+			case OPT_STRING:
    ++			case DESCRIBE_ARG_STRING:
     +				if (match_placeholder_arg_value(arg, option[i].name, &arg,
     +								&argval, &arglen)) {
     +					if (!arglen)
2:  c34c8a4f7f ! 2:  5830c69d4d pretty: add tag option to %(describe)
    @@ Documentation/pretty-formats.txt: The placeholders are:
      			  inconsistent when tags are added or removed at
      			  the same time.
      +
    -+** 'tags[=<BOOL>]': Instead of only considering annotated tags,
    ++** 'tags[=<bool>]': Instead of only considering annotated tags,
     +   consider lightweight tags as well.
      ** 'match=<pattern>': Only consider tags matching the given
         `glob(7)` pattern, excluding the "refs/tags/" prefix.
    @@ Documentation/pretty-formats.txt: insert an empty string unless we are traversin
      decoration format if `--decorate` was not already provided on the command
      line.
      
    -+The boolean options accept an optional value `[=<BOOL>]`. The values
    ++The boolean options accept an optional value `[=<bool>]`. The values
     +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
     +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
     +option is given with no value, it's enabled.
    @@ Documentation/pretty-formats.txt: insert an empty string unless we are traversin
     
      ## pretty.c ##
     @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)
    - {
      	struct {
      		char *name;
    --		enum { OPT_STRING } type;
    -+		enum { OPT_BOOL, OPT_STRING, } type;
    + 		enum {
    ++			DESCRIBE_ARG_BOOL,
    + 			DESCRIBE_ARG_STRING,
    + 		} type;
      	}  option[] = {
    -+		{ "tags", OPT_BOOL},
    - 		{ "exclude", OPT_STRING },
    - 		{ "match", OPT_STRING },
    ++		{ "tags", DESCRIBE_ARG_BOOL},
    + 		{ "exclude", DESCRIBE_ARG_STRING },
    + 		{ "match", DESCRIBE_ARG_STRING },
      	};
     @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)
      		int found = 0;
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
      
      		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
      			switch (option[i].type) {
    -+			case OPT_BOOL:
    ++			case DESCRIBE_ARG_BOOL:
     +				if (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
     +					if (optval)
     +						strvec_pushf(args, "--%s", option[i].name);
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
     +					found = 1;
     +				}
     +				break;
    - 			case OPT_STRING:
    + 			case DESCRIBE_ARG_STRING:
      				if (match_placeholder_arg_value(arg, option[i].name, &arg,
      								&argval, &arglen)) {
     
3:  b751aaf3c6 ! 3:  032513150d pretty: add abbrev option to %(describe)
    @@ Commit message
      ## Documentation/pretty-formats.txt ##
     @@ Documentation/pretty-formats.txt: The placeholders are:
      +
    - ** 'tags[=<BOOL>]': Instead of only considering annotated tags,
    + ** 'tags[=<bool>]': Instead of only considering annotated tags,
         consider lightweight tags as well.
    -+** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
    ++** 'abbrev=<number>': Instead of using the default number of hexadecimal digits
     +   (which will vary according to the number of objects in the repository with a
    -+   default of 7) of the abbreviated object name, use <n> digits, or as many digits
    -+   as needed to form a unique object name.
    ++   default of 7) of the abbreviated object name, use <number> digits, or as many
    ++   digits as needed to form a unique object name.
      ** 'match=<pattern>': Only consider tags matching the given
         `glob(7)` pattern, excluding the "refs/tags/" prefix.
      ** 'exclude=<pattern>': Do not consider tags matching the given
     
      ## pretty.c ##
     @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)
    - {
    - 	struct {
      		char *name;
    --		enum { OPT_BOOL, OPT_STRING, } type;
    -+		enum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;
    + 		enum {
    + 			DESCRIBE_ARG_BOOL,
    ++			DESCRIBE_ARG_INTEGER,
    + 			DESCRIBE_ARG_STRING,
    + 		} type;
      	}  option[] = {
    - 		{ "tags", OPT_BOOL},
    -+		{ "abbrev", OPT_INTEGER },
    - 		{ "exclude", OPT_STRING },
    - 		{ "match", OPT_STRING },
    + 		{ "tags", DESCRIBE_ARG_BOOL},
    ++		{ "abbrev", DESCRIBE_ARG_INTEGER },
    + 		{ "exclude", DESCRIBE_ARG_STRING },
    + 		{ "match", DESCRIBE_ARG_STRING },
      	};
     @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *args)
      					found = 1;
      				}
      				break;
    -+			case OPT_INTEGER:
    ++			case DESCRIBE_ARG_INTEGER:
     +				if (match_placeholder_arg_value(arg, option[i].name, &arg,
     +								&argval, &arglen)) {
     +					char *endptr;
    @@ pretty.c: static size_t parse_describe_args(const char *start, struct strvec *ar
     +					found = 1;
     +				}
     +				break;
    - 			case OPT_STRING:
    + 			case DESCRIBE_ARG_STRING:
      				if (match_placeholder_arg_value(arg, option[i].name, &arg,
      								&argval, &arglen)) {
     
-- 
2.33.1

It contains option arguments only, not options. We would like to add
option support here too, but to do that we need to distinguish between
different types of options.

Lay out the groundwork for distinguishing between bools, strings, etc.
and move the central logic (validating values and pushing new arguments
to *args) into the successful match, because that will be fairly
conditional on what type of argument is being parsed.

Signed-off-by: Eli Schwartz <redacted>
---
 pretty.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/pretty.c b/pretty.c
index be477bd51f..c38acda8cb 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1212,28 +1212,39 @@ int format_set_trailers_options(struct process_trailer_options *opts,
 
 static size_t parse_describe_args(const char *start, struct strvec *args)
 {
-	const char *options[] = { "match", "exclude" };
+	struct {
+		char *name;
+		enum {
+			DESCRIBE_ARG_STRING,
+		} type;
+	}  option[] = {
+		{ "exclude", DESCRIBE_ARG_STRING },
+		{ "match", DESCRIBE_ARG_STRING },
+	};
 	const char *arg = start;
 
 	for (;;) {
-		const char *matched = NULL;
+		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
 		int i;
 
-		for (i = 0; i < ARRAY_SIZE(options); i++) {
-			if (match_placeholder_arg_value(arg, options[i], &arg,
-							&argval, &arglen)) {
-				matched = options[i];
+		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
+			switch (option[i].type) {
+			case DESCRIBE_ARG_STRING:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen)) {
+					if (!arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
 				break;
 			}
 		}
-		if (!matched)
+		if (!found)
 			break;
 
-		if (!arglen)
-			return 0;
-		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
 	}
 	return arg - start;
 }

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, only
supports annotated tags. However, some people do use lightweight tags
for releases, and would like to describe those anyway. The command line
tool has an option to support this.

Teach the placeholder to support this as well.

Signed-off-by: Eli Schwartz <redacted>
---

I use lowercase "bool" here not "boolean-value" because I don't see
utility in the word "value" here.

 Documentation/pretty-formats.txt | 12 +++++++-----
 pretty.c                         | 12 ++++++++++++
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index ef6bd420ae..1ee47bd4a3 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -220,6 +220,8 @@ The placeholders are:
 			  inconsistent when tags are added or removed at
 			  the same time.
 +
+** 'tags[=<bool>]': Instead of only considering annotated tags,
+   consider lightweight tags as well.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

@@ -273,11 +275,6 @@ endif::git-rev-list[]
 			  If any option is provided multiple times the
 			  last occurrence wins.
 +
-The boolean options accept an optional value `[=<BOOL>]`. The values
-`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
-sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
-option is given with no value, it's enabled.
-+
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are

@@ -313,6 +310,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
 decoration format if `--decorate` was not already provided on the command
 line.
 
+The boolean options accept an optional value `[=<bool>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
+
 If you add a `+` (plus sign) after '%' of a placeholder, a line-feed
 is inserted immediately before the expansion if and only if the
 placeholder expands to a non-empty string.

diff --git a/pretty.c b/pretty.c
index c38acda8cb..403d89725a 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1215,9 +1215,11 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 	struct {
 		char *name;
 		enum {
+			DESCRIBE_ARG_BOOL,
 			DESCRIBE_ARG_STRING,
 		} type;
 	}  option[] = {
+		{ "tags", DESCRIBE_ARG_BOOL},
 		{ "exclude", DESCRIBE_ARG_STRING },
 		{ "match", DESCRIBE_ARG_STRING },
 	};

@@ -1227,10 +1229,20 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 		int found = 0;
 		const char *argval;
 		size_t arglen = 0;
+		int optval = 0;
 		int i;
 
 		for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
 			switch (option[i].type) {
+			case DESCRIBE_ARG_BOOL:
+				if (match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
+					if (optval)
+						strvec_pushf(args, "--%s", option[i].name);
+					else
+						strvec_pushf(args, "--no-%s", option[i].name);
+					found = 1;
+				}
+				break;
 			case DESCRIBE_ARG_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen)) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5865daa8f8..d4acf8882f 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:tags) vs git describe --tags' '
+	test_when_finished "git tag -d tagname" &&
+	git tag tagname &&
+	git describe --tags >expect &&
+	git log -1 --format="%(describe:tags)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

The %(describe) placeholder by default, like `git describe`, uses a
seven-character abbreviated commit object name. This may not be
sufficient to fully describe all commits in a given repository,
resulting in a placeholder replacement changing its length because the
repository grew in size.  This could cause the output of git-archive to
change.

Add the --abbrev option to `git describe` to the placeholder interface
in order to provide tools to the user for fine-tuning project defaults
and ensure reproducible archives.

One alternative would be to just always specify --abbrev=40 but this may
be a bit too biased...

Signed-off-by: Eli Schwartz <redacted>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         | 15 +++++++++++++++
 t/t4205-log-pretty-formats.sh    |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 1ee47bd4a3..9e943fb74b 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt

@@ -222,6 +222,10 @@ The placeholders are:
 +
 ** 'tags[=<bool>]': Instead of only considering annotated tags,
    consider lightweight tags as well.
+** 'abbrev=<number>': Instead of using the default number of hexadecimal digits
+   (which will vary according to the number of objects in the repository with a
+   default of 7) of the abbreviated object name, use <number> digits, or as many
+   digits as needed to form a unique object name.
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
 ** 'exclude=<pattern>': Do not consider tags matching the given

diff --git a/pretty.c b/pretty.c
index 403d89725a..fa9bfea273 100644
--- a/pretty.c
+++ b/pretty.c

@@ -1216,10 +1216,12 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 		char *name;
 		enum {
 			DESCRIBE_ARG_BOOL,
+			DESCRIBE_ARG_INTEGER,
 			DESCRIBE_ARG_STRING,
 		} type;
 	}  option[] = {
 		{ "tags", DESCRIBE_ARG_BOOL},
+		{ "abbrev", DESCRIBE_ARG_INTEGER },
 		{ "exclude", DESCRIBE_ARG_STRING },
 		{ "match", DESCRIBE_ARG_STRING },
 	};

@@ -1243,6 +1245,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
 					found = 1;
 				}
 				break;
+			case DESCRIBE_ARG_INTEGER:
+				if (match_placeholder_arg_value(arg, option[i].name, &arg,
+								&argval, &arglen)) {
+					char *endptr;
+					if (!arglen)
+						return 0;
+					strtol(argval, &endptr, 10);
+					if (endptr - argval != arglen)
+						return 0;
+					strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
+					found = 1;
+				}
+				break;
 			case DESCRIBE_ARG_STRING:
 				if (match_placeholder_arg_value(arg, option[i].name, &arg,
 								&argval, &arglen)) {

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d4acf8882f..35eef4c865 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh

@@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
+	test_when_finished "git tag -d tagname" &&
+	git tag -a -m tagged tagname &&
+	git describe --abbrev=15 >expect &&
+	git log -1 --format="%(describe:abbrev=15)" >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.33.1

Hi Eli,

On Tue, 26 Oct 2021, Eli Schwartz wrote:

On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:

quoted

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

Thanks for the advice. In v1 of this patchset I attempted to do a
developer build but failed due to preexisting errors:


    CC run-command.o
run-command.c: In function ‘async_die_is_recursing’:
run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a
region of size 0 [-Werror=stringop-overread]
 1102 |         pthread_setspecific(async_die_counter, (void *)1);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/openssl/crypto.h:415,
                 from /usr/include/openssl/comp.h:16,
                 from /usr/include/openssl/ssl.h:17,
                 from git-compat-util.h:309,
                 from cache.h:4,
                 from run-command.c:1:
/usr/include/pthread.h:1308:12: note: in a call to function
‘pthread_setspecific’ declared with attribute ‘access (none, 2)’
 1308 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors



My system has a custom compiled glibc from git roughly around the 2.34
release (a similar environment could be obtained by using Fedora rawhide
I guess), and this commit looks mighty suspicious:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0

For this reason, I did not bother to try testing v2 under a developer
build, leading to my overlooking this issue. ;)

It seems that this issue now hit an official version. As I explained in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,
my colleague Victoria Dye will send a fix for this later.

Stay tuned,
Johannes

Hi Eli,

On Thu, 4 Nov 2021, Johannes Schindelin wrote:

On Tue, 26 Oct 2021, Eli Schwartz wrote:

quoted

On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:

quoted

Other than the question pointed out by Eric,

with DEVELOPER=1, -Werror=declaration-after-statement
We'll need this change squashed in:

Thanks for the advice. In v1 of this patchset I attempted to do a
developer build but failed due to preexisting errors:


    CC run-command.o
run-command.c: In function ‘async_die_is_recursing’:
run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a
region of size 0 [-Werror=stringop-overread]
 1102 |         pthread_setspecific(async_die_counter, (void *)1);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/openssl/crypto.h:415,
                 from /usr/include/openssl/comp.h:16,
                 from /usr/include/openssl/ssl.h:17,
                 from git-compat-util.h:309,
                 from cache.h:4,
                 from run-command.c:1:
/usr/include/pthread.h:1308:12: note: in a call to function
‘pthread_setspecific’ declared with attribute ‘access (none, 2)’
 1308 | extern int pthread_setspecific (pthread_key_t __key,
      |            ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors



My system has a custom compiled glibc from git roughly around the 2.34
release (a similar environment could be obtained by using Fedora rawhide
I guess), and this commit looks mighty suspicious:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0

For this reason, I did not bother to try testing v2 under a developer
build, leading to my overlooking this issue. ;)

It seems that this issue now hit an official version. As I explained in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,
my colleague Victoria Dye will send a fix for this later.

FYI here is the patch:
https://lore.kernel.org/git/pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com/

Ciao,
Johannes

On 11/3/21 7:20 PM, Johannes Schindelin wrote:

Hi Eli,

On Tue, 26 Oct 2021, Eli Schwartz wrote:

quoted

My system has a custom compiled glibc from git roughly around the 2.34
release (a similar environment could be obtained by using Fedora rawhide
I guess), and this commit looks mighty suspicious:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0

For this reason, I did not bother to try testing v2 under a developer
build, leading to my overlooking this issue. ;)

It seems that this issue now hit an official version. As I explained in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,
my colleague Victoria Dye will send a fix for this later.

Stay tuned,

FWIW this was present in the official version of glibc released in
August... the problem is finding an official version of a distro that
ships it. :D

Thanks for the heads up.


-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User