gitattributes export-subst and software versioning

gitattributes export-subst and software versioning

[Eli Schwartz]

Periodically, I wonder if there is some better way for managing tagged 
releases for software in git. Current state of the art seems to be 
"write a custom Makefile that takes a version and seds out the existing 
version, then runs git tag for you". Inelegant solutions also abound; 
people release code that does not build properly unless you build it 
from a git checkout so it can run git describe. (There are half a dozen 
individually popular but mutually exclusive python ecosystems for this, 
in fact, all of them varying degrees of broken.)

git does have a way to automatically insert metadata via the 
export-subst attribute on a file, but it's very awkward to use and you 
cannot get much info out of it.

# get tags into a file, only on exact match:

$ cat VERSION
$Format:%d$
$Format:%D$

$ git archive HEAD | bsdtar -xOf - VERSION
   (HEAD -> master, tag: 1.0)
HEAD -> master, tag: 1.0

With sufficient regex, you can get a release out of this, but it doesn't 
work if you try getting an autogenerated tarball for a commit that isn't 
exactly a release.

$ git commit --allow-empty -m ...
$ git archive HEAD | bsdtar -xOf - VERSION
   (HEAD -> master)
HEAD -> master

I think it would be much, much nicer if there was a format placeholder 
for git describe.

It doesn't even need option support -- the default output in most cases 
could be a replacement for or fall back to existing invocations of the 
"git" program, followed by post-processing with e.g. "sed".

However, the existence of current pretty formats such as %C() or 
%(trailer:options) implies that options could be passed in a 
git-describe format too. e.g. %(describe:--long --tags --match="v*")

Thoughts?

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

Re: gitattributes export-subst and software versioning

[René Scharfe]

Am 25.01.21 um 01:32 schrieb Eli Schwartz:
> Periodically, I wonder if there is some better way for managing
> tagged releases for software in git. Current state of the art seems
> to be "write a custom Makefile that takes a version and seds out the
> existing version, then runs git tag for you". Inelegant solutions
> also abound; people release code that does not build properly unless
> you build it from a git checkout so it can run git describe. (There
> are half a dozen individually popular but mutually exclusive python
> ecosystems for this, in fact, all of them varying degrees of
> broken.)
>
> git does have a way to automatically insert metadata via the
> export-subst attribute on a file, but it's very awkward to use and
> you cannot get much info out of it.
>
> # get tags into a file, only on exact match:
>
> $ cat VERSION $Format:%d$ $Format:%D$
>
> $ git archive HEAD | bsdtar -xOf - VERSION (HEAD -> master, tag:
> 1.0) HEAD -> master, tag: 1.0
>
> With sufficient regex, you can get a release out of this, but it
> doesn't work if you try getting an autogenerated tarball for a commit
> that isn't exactly a release.
>
> $ git commit --allow-empty -m ... $ git archive HEAD | bsdtar -xOf -
> VERSION (HEAD -> master) HEAD -> master
>
> I think it would be much, much nicer if there was a format
> placeholder for git describe.

Totally.

> It doesn't even need option support -- the default output in most
> cases could be a replacement for or fall back to existing invocations
> of the "git" program, followed by post-processing with e.g. "sed".
>
> However, the existence of current pretty formats such as %C() or
> %(trailer:options) implies that options could be passed in a
> git-describe format too. e.g. %(describe:--long --tags --match="v*")
>
> Thoughts?

git archive uses the pretty format code for export-subst.  It is used by
git log and others as well.  git describe uses all object flags to find
the best description.  Simply plugging it into the pretty format code
would clash with the object flag use of git log.

And replacing the flags with a commit slab doesn't seem to be enough,
either -- I get good results lots of commits, but for some git log with
the new placeholder would just show some nonsensical output, as it
seems to get the depth calculation wrong for them somehow.

Anyway, we can of course do something like in the patch below.  It
works, it's easy, it's fast enough for git archive, and it's quite
hideous.  Hopefully it's bad enough to motivate someone to come up with
a cleaner, faster solution.

René


---
 pretty.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/pretty.c b/pretty.c
index 3922f6f9f2..bbfb5ca3e7 100644
--- a/pretty.c
+++ b/pretty.c
@@ -12,6 +12,7 @@
 #include "reflog-walk.h"
 #include "gpg-interface.h"
 #include "trailer.h"
+#include "run-command.h"

 static char *user_format;
 static struct cmt_fmt_map {
@@ -1213,6 +1214,21 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return parse_padding_placeholder(placeholder, c);
 	}

+	if (skip_prefix(placeholder, "(describe)", &arg)) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_push(&cmd.args, "describe");
+		strvec_push(&cmd.args, "--always");
+		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+		pipe_command(&cmd, NULL, 0, &out, 0, NULL, 0);
+		strbuf_rtrim(&out);
+		strbuf_addbuf(sb, &out);
+		strbuf_release(&out);
+		return arg - placeholder;
+	}
+
 	/* these depend on the commit */
 	if (!commit->object.parsed)
 		parse_object(the_repository, &commit->object.oid);
--
2.30.0

Re: gitattributes export-subst and software versioning

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Anyway, we can of course do something like in the patch below.  It
> works, it's easy, it's fast enough for git archive, and it's quite
> hideous.  Hopefully it's bad enough to motivate someone to come up with
> a cleaner, faster solution.

Yeah, I can see it would work, it is no-brainer-easy, it is clean,
and I can believe it would be fast enough and overall, the best
implementation for the purpose of "git archive".

Perhaps not for "log", but then I do not know if there is a good way
to have it natively run inside "log" without major surgery, as the
"describe" operation itself is fairly resource intensive to remember
which tagged commit is the closest to compute description for even a
single commit.

Re: gitattributes export-subst and software versioning

[Eli Schwartz]

[-- Attachment #1.1: Type: text/plain, Size: 3071 bytes --]

On 2/8/21 2:46 PM, René Scharfe wrote:
> git archive uses the pretty format code for export-subst.  It is used by
> git log and others as well.  git describe uses all object flags to find
> the best description.  Simply plugging it into the pretty format code
> would clash with the object flag use of git log.

Yeah, I was afraid there might be bad interactions with a pretty
archive-centric placeholder and something that isn't git-archive.

On the other hand, with my zero knowledge of the code but having read
lots of man pages... %S documents that it "only works with git log", so
maybe it is possible to add an option that is documented to only work
for git archive?

e.g. if you do use it,

$ cat VERSION
$Format:%d$
$Format:%S$

$ git archive HEAD | bsdtar -xOf - VERSION
 (HEAD -> master, tag: 1.0)
%S

It's apparently completely ignored and treated as raw characters. The
same restriction could theoretically be added in the other direction for
a new placeholder.

This would neatly resolve Junio's concern about the resource-intensive
nature of "describe" not being a good fit for "log".

> And replacing the flags with a commit slab doesn't seem to be enough,
> either -- I get good results lots of commits, but for some git log with
> the new placeholder would just show some nonsensical output, as it
> seems to get the depth calculation wrong for them somehow.

You mean git describe <commit> produces wrong results for those?

> Anyway, we can of course do something like in the patch below.  It
> works, it's easy, it's fast enough for git archive, and it's quite
> hideous.  Hopefully it's bad enough to motivate someone to come up with
> a cleaner, faster solution.

:D :D an important part of the resolution process, to be sure.

> ---
>  pretty.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/pretty.c b/pretty.c
> index 3922f6f9f2..bbfb5ca3e7 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -12,6 +12,7 @@
>  #include "reflog-walk.h"
>  #include "gpg-interface.h"
>  #include "trailer.h"
> +#include "run-command.h"
> 
>  static char *user_format;
>  static struct cmt_fmt_map {
> @@ -1213,6 +1214,21 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  		return parse_padding_placeholder(placeholder, c);
>  	}
> 
> +	if (skip_prefix(placeholder, "(describe)", &arg)) {
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +		struct strbuf out = STRBUF_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_push(&cmd.args, "describe");
> +		strvec_push(&cmd.args, "--always");
> +		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
> +		pipe_command(&cmd, NULL, 0, &out, 0, NULL, 0);
> +		strbuf_rtrim(&out);
> +		strbuf_addbuf(sb, &out);
> +		strbuf_release(&out);
> +		return arg - placeholder;
> +	}
> +
>  	/* these depend on the commit */
>  	if (!commit->object.parsed)
>  		parse_object(the_repository, &commit->object.oid);
> --
> 2.30.0
> 


-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: gitattributes export-subst and software versioning

[Junio C Hamano]

Eli Schwartz <eschwartz@archlinux.org> writes:

>> And replacing the flags with a commit slab doesn't seem to be enough,
>> either -- I get good results lots of commits, but for some git log with
>> the new placeholder would just show some nonsensical output, as it
>> seems to get the depth calculation wrong for them somehow.
>
> You mean git describe <commit> produces wrong results for those?

Running "describe" as an external command, like the patch you are
responding to does, would not produce any wrong result, but Réne's
point is that it would be very involved to turn "describe" into a
function that can be called as if it is a library function without
disrupting the operation of "log" that is running in the same
address space, and the change he tried had some weird bugs to get
the computation wrong.

Re: gitattributes export-subst and software versioning

[René Scharfe]

Am 09.02.21 um 21:42 schrieb Junio C Hamano:
> Eli Schwartz <eschwartz@archlinux.org> writes:
>
>>> And replacing the flags with a commit slab doesn't seem to be enough,
>>> either -- I get good results lots of commits, but for some git log with
>>> the new placeholder would just show some nonsensical output, as it
>>> seems to get the depth calculation wrong for them somehow.
>>
>> You mean git describe <commit> produces wrong results for those?
>
> Running "describe" as an external command, like the patch you are
> responding to does, would not produce any wrong result, but Réne's
> point is that it would be very involved to turn "describe" into a
> function that can be called as if it is a library function without
> disrupting the operation of "log" that is running in the same
> address space, and the change he tried had some weird bugs to get
> the computation wrong.

Yes, indeed.  Let's go with the forking version for now -- it may be
slow, but it's fast enough, and it's better than nothing.

René

Re: gitattributes export-subst and software versioning

[René Scharfe]

Am 09.02.21 um 01:19 schrieb Eli Schwartz:
> On the other hand, with my zero knowledge of the code but having read
> lots of man pages... %S documents that it "only works with git log", so
> maybe it is possible to add an option that is documented to only work
> for git archive?
>
> e.g. if you do use it,
>
> $ cat VERSION
> $Format:%d$
> $Format:%S$
>
> $ git archive HEAD | bsdtar -xOf - VERSION
>  (HEAD -> master, tag: 1.0)
> %S
>
> It's apparently completely ignored and treated as raw characters. The
> same restriction could theoretically be added in the other direction for
> a new placeholder.

That's a curious case.  Supporting %S in git archive would be easy  --
it's just a matter of recording the name given at the command line for
the pretty format code to find, like in the sloppy patch below (missing
doc update, missing test, leaks memory, adds a static variable to
library code).

The inconsistent support of %S is not ideal and I think that set a bad
precedent, but on the other hand I don't see its usefulness for git
archive in particular.  So I dunno.  Perhaps worth doing once rev-list
gains support, for completeness.

René

---
 archive.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/archive.c b/archive.c
index 5919d9e505..ffe4c35961 100644
--- a/archive.c
+++ b/archive.c
@@ -9,6 +9,7 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "dir.h"
+#include "revision.h"

 static char const * const archive_usage[] = {
 	N_("git archive [<options>] <tree-ish> [<path>...]"),
@@ -23,6 +24,8 @@ static int nr_archivers;
 static int alloc_archivers;
 static int remote_allow_unreachable;

+static struct revision_sources revision_sources;
+
 void register_archiver(struct archiver *ar)
 {
 	ALLOC_GROW(archivers, nr_archivers + 1, alloc_archivers);
@@ -41,7 +44,8 @@ static void format_subst(const struct commit *commit,
 {
 	char *to_free = NULL;
 	struct strbuf fmt = STRBUF_INIT;
-	struct pretty_print_context ctx = {0};
+	struct rev_info rev = { .sources = &revision_sources };
+	struct pretty_print_context ctx = { .rev = &rev };
 	ctx.date_mode.type = DATE_NORMAL;
 	ctx.abbrev = DEFAULT_ABBREV;

@@ -461,6 +465,8 @@ static void parse_treeish_arg(const char **argv,

 	commit = lookup_commit_reference_gently(ar_args->repo, &oid, 1);
 	if (commit) {
+		init_revision_sources(&revision_sources);
+		*revision_sources_at(&revision_sources, commit) = xstrdup(name);
 		commit_oid = &commit->object.oid;
 		archive_time = commit->date;
 	} else {
--
2.30.1

[PATCH 1/2] pretty: add %(describe)

[René Scharfe]

Add a format placeholder for describe output.  Implement it by actually
calling git describe, which is simple and guarantees correctness.  It's
intended to be used with $Format:...$ in files with the attribute
export-subst and git archive.  It can also be used with git log etc.,
even though that's going to be slow due to the fork for each commit.

Suggested-by: Eli Schwartz <eschwartz@archlinux.org>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/pretty-formats.txt |  2 ++
 pretty.c                         | 17 +++++++++++++++++
 t/t4205-log-pretty-formats.sh    | 10 ++++++++++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6b59e28d44..bb8c05bc21 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -208,6 +208,8 @@ The placeholders are:
 '%cs':: committer date, short format (`YYYY-MM-DD`)
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
+'%(describe)':: human-readable name, like linkgit:git-describe[1];
+		empty string for undescribable commits
 '%S':: ref name given on the command line by which the commit was reached
        (like `git log --source`), only works with `git log`
 '%e':: encoding
diff --git a/pretty.c b/pretty.c
index b4ff3f602f..a0c427fb61 100644
--- a/pretty.c
+++ b/pretty.c
@@ -12,6 +12,7 @@
 #include "reflog-walk.h"
 #include "gpg-interface.h"
 #include "trailer.h"
+#include "run-command.h"

 static char *user_format;
 static struct cmt_fmt_map {
@@ -1214,6 +1215,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return parse_padding_placeholder(placeholder, c);
 	}

+	if (skip_prefix(placeholder, "(describe)", &arg)) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+		struct strbuf err = STRBUF_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_push(&cmd.args, "describe");
+		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
+		pipe_command(&cmd, NULL, 0, &out, 0, &err, 0);
+		strbuf_rtrim(&out);
+		strbuf_addbuf(sb, &out);
+		strbuf_release(&out);
+		strbuf_release(&err);
+		return arg - placeholder;
+	}
+
 	/* these depend on the commit */
 	if (!commit->object.parsed)
 		parse_object(the_repository, &commit->object.oid);
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 749bc1431a..5a44fa447d 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -962,4 +962,14 @@ test_expect_success 'log --pretty=reference is colored appropriately' '
 	test_cmp expect actual
 '

+test_expect_success '%(describe) vs git describe' '
+	git log --format="%H" | while read hash
+	do
+		echo "$hash $(git describe $hash)"
+	done >expect &&
+	git log --format="%H %(describe)" >actual 2>err &&
+	test_cmp expect actual &&
+	test_must_be_empty err
+'
+
 test_done
--
2.30.1

[PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Allow restricting the tags used by the placeholder %(describe) with the
options match and exclude.  E.g. the following command describes the
current commit using official version tags, without those for release
candidates:

   $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/pretty-formats.txt | 13 ++++++++--
 pretty.c                         | 43 ++++++++++++++++++++++++++++++--
 t/t4205-log-pretty-formats.sh    | 16 ++++++++++++
 3 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index bb8c05bc21..231010e6ef 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -208,8 +208,17 @@ The placeholders are:
 '%cs':: committer date, short format (`YYYY-MM-DD`)
 '%d':: ref names, like the --decorate option of linkgit:git-log[1]
 '%D':: ref names without the " (", ")" wrapping.
-'%(describe)':: human-readable name, like linkgit:git-describe[1];
-		empty string for undescribable commits
+'%(describe[:options])':: human-readable name, like
+			  linkgit:git-describe[1]; empty string for
+			  undescribable commits.  The `describe` string
+			  may be followed by a colon and zero or more
+			  comma-separated options.
++
+** '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
+   `glob(7)` pattern, excluding the "refs/tags/" prefix.
+
 '%S':: ref name given on the command line by which the commit was reached
        (like `git log --source`), only works with `git log`
 '%e':: encoding
diff --git a/pretty.c b/pretty.c
index a0c427fb61..c612d2ac9b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1150,6 +1150,34 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 	return 0;
 }

+static size_t parse_describe_args(const char *start, struct strvec *args)
+{
+	const char *options[] = { "match", "exclude" };
+	const char *arg = start;
+
+	for (;;) {
+		const char *matched = NULL;
+		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];
+				break;
+			}
+		}
+		if (!matched)
+			break;
+
+		if (!arglen)
+			return 0;
+		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
+	}
+	return arg - start;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1215,20 +1243,31 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return parse_padding_placeholder(placeholder, c);
 	}

-	if (skip_prefix(placeholder, "(describe)", &arg)) {
+	if (skip_prefix(placeholder, "(describe", &arg)) {
 		struct child_process cmd = CHILD_PROCESS_INIT;
 		struct strbuf out = STRBUF_INIT;
 		struct strbuf err = STRBUF_INIT;

 		cmd.git_cmd = 1;
 		strvec_push(&cmd.args, "describe");
+
+		if (*arg == ':') {
+			arg++;
+			arg += parse_describe_args(arg, &cmd.args);
+		}
+
+		if (*arg != ')') {
+			child_process_clear(&cmd);
+			return 0;
+		}
+
 		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
 		pipe_command(&cmd, NULL, 0, &out, 0, &err, 0);
 		strbuf_rtrim(&out);
 		strbuf_addbuf(sb, &out);
 		strbuf_release(&out);
 		strbuf_release(&err);
-		return arg - placeholder;
+		return arg - placeholder + 1;
 	}

 	/* these depend on the commit */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5a44fa447d..7e36706212 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -972,4 +972,20 @@ test_expect_success '%(describe) vs git describe' '
 	test_must_be_empty err
 '

+test_expect_success '%(describe:match=...) vs git describe --match ...' '
+	test_when_finished "git tag -d tag-match" &&
+	git tag -a -m tagged tag-match&&
+	git describe --match "*-match" >expect &&
+	git log -1 --format="%(describe:match=*-match)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
+	test_when_finished "git tag -d tag-exclude" &&
+	git tag -a -m tagged tag-exclude &&
+	git describe --exclude "*-exclude" >expect &&
+	git log -1 --format="%(describe:exclude=*-exclude)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
--
2.30.1

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[Jeff King]

On Sun, Feb 14, 2021 at 11:10:57AM +0100, René Scharfe. wrote:

> Allow restricting the tags used by the placeholder %(describe) with the
> options match and exclude.  E.g. the following command describes the
> current commit using official version tags, without those for release
> candidates:
> 
>    $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'

An interesting side effect of this series is that it allows remote users
asking for archives to fill in this data, too (by using export-subst
placeholders). That includes servers allowing "git archive --remote",
but also services like GitHub that will run git-archive on behalf of
clients.

I wonder what avenues for mischief this provides. Certainly using extra
CPU to run git-describe. But I guess also probing at otherwise hidden
refs using the match/exclude system (though since it's limited to
refs/tags/, that's pretty unlikely).

I present this mostly as an observation, not an objection. Certainly we
already have %D which can look at hidden refs. And I strongly suspect
that the server-side git-upload-archive does not respect hidden refs
when resolving object names in the first place.

Kind of an interesting thought as we extend the formatting language,
though. I generally think of them as something that is always under
control of caller, but export-subst's $Format$ will come from the repo
contents.

-Peff

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Am 17.02.21 um 19:31 schrieb Jeff King:
> On Sun, Feb 14, 2021 at 11:10:57AM +0100, René Scharfe. wrote:
>
>> Allow restricting the tags used by the placeholder %(describe) with the
>> options match and exclude.  E.g. the following command describes the
>> current commit using official version tags, without those for release
>> candidates:
>>
>>    $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'
>
> An interesting side effect of this series is that it allows remote users
> asking for archives to fill in this data, too (by using export-subst
> placeholders). That includes servers allowing "git archive --remote",
> but also services like GitHub that will run git-archive on behalf of
> clients.
>
> I wonder what avenues for mischief this provides. Certainly using extra
> CPU to run git-describe.

A repository can contain millions of files, each file can contain
millions of $Format:...$ sequences and each of them can contain millions
of %(describe) placeholders.  Each of them could have different match or
exclude args to prevent caching.  Allowing a single request to cause
trillions of calls of git describe sounds excessive.  Let's limit this.

-- >8 --
Subject: [PATCH] archive: expand only a single %(describe) per archive

Every %(describe) placeholder in $Format:...$ strings in files with the
attribute export-subst is expanded by calling git describe.  This can
potentially result in a lot of such calls per archive.  That's OK for
local repositories under control of the user of git archive, but could
be a problem for hosted repositories.

Expand only a single %(describe) placeholder per archive for now to
avoid denial-of-service attacks.  We can make this limit configurable
later if needed, but let's start out simple.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/gitattributes.txt |  3 ++-
 archive.c                       | 16 ++++++++++------
 archive.h                       |  2 ++
 pretty.c                        |  8 ++++++++
 pretty.h                        |  5 +++++
 t/t5001-archive-attr.sh         | 14 ++++++++++++++
 6 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e84e104f93..0a60472bb5 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -1174,7 +1174,8 @@ tag then no replacement will be done.  The placeholders are the same
 as those for the option `--pretty=format:` of linkgit:git-log[1],
 except that they need to be wrapped like this: `$Format:PLACEHOLDERS$`
 in the file.  E.g. the string `$Format:%H$` will be replaced by the
-commit hash.
+commit hash.  However, only one `%(describe)` placeholder is expanded
+per archive to avoid denial-of-service attacks.


 Packing objects
diff --git a/archive.c b/archive.c
index 5919d9e505..2dd2236ae0 100644
--- a/archive.c
+++ b/archive.c
@@ -37,13 +37,10 @@ void init_archivers(void)

 static void format_subst(const struct commit *commit,
 			 const char *src, size_t len,
-			 struct strbuf *buf)
+			 struct strbuf *buf, struct pretty_print_context *ctx)
 {
 	char *to_free = NULL;
 	struct strbuf fmt = STRBUF_INIT;
-	struct pretty_print_context ctx = {0};
-	ctx.date_mode.type = DATE_NORMAL;
-	ctx.abbrev = DEFAULT_ABBREV;

 	if (src == buf->buf)
 		to_free = strbuf_detach(buf, NULL);
@@ -61,7 +58,7 @@ static void format_subst(const struct commit *commit,
 		strbuf_add(&fmt, b + 8, c - b - 8);

 		strbuf_add(buf, src, b - src);
-		format_commit_message(commit, fmt.buf, buf, &ctx);
+		format_commit_message(commit, fmt.buf, buf, ctx);
 		len -= c + 1 - src;
 		src  = c + 1;
 	}
@@ -94,7 +91,7 @@ static void *object_file_to_archive(const struct archiver_args *args,
 		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
 		convert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);
 		if (commit)
-			format_subst(commit, buf.buf, buf.len, &buf);
+			format_subst(commit, buf.buf, buf.len, &buf, args->pretty_ctx);
 		buffer = strbuf_detach(&buf, &size);
 		*sizep = size;
 	}
@@ -633,12 +630,19 @@ int write_archive(int argc, const char **argv, const char *prefix,
 		  const char *name_hint, int remote)
 {
 	const struct archiver *ar = NULL;
+	struct pretty_print_describe_status describe_status = {0};
+	struct pretty_print_context ctx = {0};
 	struct archiver_args args;
 	int rc;

 	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
 	git_config(git_default_config, NULL);

+	describe_status.max_invocations = 1;
+	ctx.date_mode.type = DATE_NORMAL;
+	ctx.abbrev = DEFAULT_ABBREV;
+	ctx.describe_status = &describe_status;
+	args.pretty_ctx = &ctx;
 	args.repo = repo;
 	args.prefix = prefix;
 	string_list_init(&args.extra_files, 1);
diff --git a/archive.h b/archive.h
index 33551b7ee1..49fab71aaf 100644
--- a/archive.h
+++ b/archive.h
@@ -5,6 +5,7 @@
 #include "pathspec.h"

 struct repository;
+struct pretty_print_context;

 struct archiver_args {
 	struct repository *repo;
@@ -22,6 +23,7 @@ struct archiver_args {
 	unsigned int convert : 1;
 	int compression_level;
 	struct string_list extra_files;
+	struct pretty_print_context *pretty_ctx;
 };

 /* main api */
diff --git a/pretty.c b/pretty.c
index c612d2ac9b..032e89cd4e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1247,6 +1247,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		struct child_process cmd = CHILD_PROCESS_INIT;
 		struct strbuf out = STRBUF_INIT;
 		struct strbuf err = STRBUF_INIT;
+		struct pretty_print_describe_status *describe_status;
+
+		describe_status = c->pretty_ctx->describe_status;
+		if (describe_status) {
+			if (!describe_status->max_invocations)
+				return 0;
+			describe_status->max_invocations--;
+		}

 		cmd.git_cmd = 1;
 		strvec_push(&cmd.args, "describe");
diff --git a/pretty.h b/pretty.h
index 7ce6c0b437..27b15947ff 100644
--- a/pretty.h
+++ b/pretty.h
@@ -23,6 +23,10 @@ enum cmit_fmt {
 	CMIT_FMT_UNSPECIFIED
 };

+struct pretty_print_describe_status {
+	unsigned int max_invocations;
+};
+
 struct pretty_print_context {
 	/*
 	 * Callers should tweak these to change the behavior of pp_* functions.
@@ -44,6 +48,7 @@ struct pretty_print_context {
 	int color;
 	struct ident_split *from_ident;
 	unsigned encode_email_headers:1;
+	struct pretty_print_describe_status *describe_status;

 	/*
 	 * Fields below here are manipulated internally by pp_* functions and
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index e9aa97117a..1c9ce3956b 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -128,4 +128,18 @@ test_expect_success 'export-subst' '
 	test_cmp substfile2 archive/substfile2
 '

+test_expect_success 'export-subst expands %(describe) once' '
+	echo "\$Format:%(describe)\$" >substfile3 &&
+	echo "\$Format:%(describe)\$" >>substfile3 &&
+	echo "\$Format:%(describe)${LF}%(describe)\$" >substfile4 &&
+	git add substfile[34] &&
+	git commit -m export-subst-describe &&
+	git tag -m export-subst-describe export-subst-describe &&
+	git archive HEAD >archive-describe.tar &&
+	extract_tar_to_dir archive-describe &&
+	desc=$(git describe) &&
+	grep -F "$desc" archive-describe/substfile[34] >substituted &&
+	test_line_count = 1 substituted
+'
+
 test_done
--
2.30.1

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 28 2021, René Scharfe. wrote:

> Am 17.02.21 um 19:31 schrieb Jeff King:
>> On Sun, Feb 14, 2021 at 11:10:57AM +0100, René Scharfe. wrote:
>>
>>> Allow restricting the tags used by the placeholder %(describe) with the
>>> options match and exclude.  E.g. the following command describes the
>>> current commit using official version tags, without those for release
>>> candidates:
>>>
>>>    $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'
>>
>> An interesting side effect of this series is that it allows remote users
>> asking for archives to fill in this data, too (by using export-subst
>> placeholders). That includes servers allowing "git archive --remote",
>> but also services like GitHub that will run git-archive on behalf of
>> clients.
>>
>> I wonder what avenues for mischief this provides. Certainly using extra
>> CPU to run git-describe.
>
> A repository can contain millions of files, each file can contain
> millions of $Format:...$ sequences and each of them can contain millions
> of %(describe) placeholders.  Each of them could have different match or
> exclude args to prevent caching.  Allowing a single request to cause
> trillions of calls of git describe sounds excessive.  Let's limit this.
>
> -- >8 --
> Subject: [PATCH] archive: expand only a single %(describe) per archive
>
> Every %(describe) placeholder in $Format:...$ strings in files with the
> attribute export-subst is expanded by calling git describe.  This can
> potentially result in a lot of such calls per archive.  That's OK for
> local repositories under control of the user of git archive, but could
> be a problem for hosted repositories.
>
> Expand only a single %(describe) placeholder per archive for now to
> avoid denial-of-service attacks.  We can make this limit configurable
> later if needed, but let's start out simple.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/gitattributes.txt |  3 ++-
>  archive.c                       | 16 ++++++++++------
>  archive.h                       |  2 ++
>  pretty.c                        |  8 ++++++++
>  pretty.h                        |  5 +++++
>  t/t5001-archive-attr.sh         | 14 ++++++++++++++
>  6 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index e84e104f93..0a60472bb5 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -1174,7 +1174,8 @@ tag then no replacement will be done.  The placeholders are the same
>  as those for the option `--pretty=format:` of linkgit:git-log[1],
>  except that they need to be wrapped like this: `$Format:PLACEHOLDERS$`
>  in the file.  E.g. the string `$Format:%H$` will be replaced by the
> -commit hash.
> +commit hash.  However, only one `%(describe)` placeholder is expanded
> +per archive to avoid denial-of-service attacks.
>
>
>  Packing objects
> diff --git a/archive.c b/archive.c
> index 5919d9e505..2dd2236ae0 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -37,13 +37,10 @@ void init_archivers(void)
>
>  static void format_subst(const struct commit *commit,
>  			 const char *src, size_t len,
> -			 struct strbuf *buf)
> +			 struct strbuf *buf, struct pretty_print_context *ctx)
>  {
>  	char *to_free = NULL;
>  	struct strbuf fmt = STRBUF_INIT;
> -	struct pretty_print_context ctx = {0};
> -	ctx.date_mode.type = DATE_NORMAL;
> -	ctx.abbrev = DEFAULT_ABBREV;
>
>  	if (src == buf->buf)
>  		to_free = strbuf_detach(buf, NULL);
> @@ -61,7 +58,7 @@ static void format_subst(const struct commit *commit,
>  		strbuf_add(&fmt, b + 8, c - b - 8);
>
>  		strbuf_add(buf, src, b - src);
> -		format_commit_message(commit, fmt.buf, buf, &ctx);
> +		format_commit_message(commit, fmt.buf, buf, ctx);
>  		len -= c + 1 - src;
>  		src  = c + 1;
>  	}
> @@ -94,7 +91,7 @@ static void *object_file_to_archive(const struct archiver_args *args,
>  		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
>  		convert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);
>  		if (commit)
> -			format_subst(commit, buf.buf, buf.len, &buf);
> +			format_subst(commit, buf.buf, buf.len, &buf, args->pretty_ctx);
>  		buffer = strbuf_detach(&buf, &size);
>  		*sizep = size;
>  	}
> @@ -633,12 +630,19 @@ int write_archive(int argc, const char **argv, const char *prefix,
>  		  const char *name_hint, int remote)
>  {
>  	const struct archiver *ar = NULL;
> +	struct pretty_print_describe_status describe_status = {0};
> +	struct pretty_print_context ctx = {0};
>  	struct archiver_args args;
>  	int rc;
>
>  	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
>  	git_config(git_default_config, NULL);
>
> +	describe_status.max_invocations = 1;
> +	ctx.date_mode.type = DATE_NORMAL;
> +	ctx.abbrev = DEFAULT_ABBREV;
> +	ctx.describe_status = &describe_status;
> +	args.pretty_ctx = &ctx;
>  	args.repo = repo;
>  	args.prefix = prefix;
>  	string_list_init(&args.extra_files, 1);
> diff --git a/archive.h b/archive.h
> index 33551b7ee1..49fab71aaf 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -5,6 +5,7 @@
>  #include "pathspec.h"
>
>  struct repository;
> +struct pretty_print_context;
>
>  struct archiver_args {
>  	struct repository *repo;
> @@ -22,6 +23,7 @@ struct archiver_args {
>  	unsigned int convert : 1;
>  	int compression_level;
>  	struct string_list extra_files;
> +	struct pretty_print_context *pretty_ctx;
>  };
>
>  /* main api */
> diff --git a/pretty.c b/pretty.c
> index c612d2ac9b..032e89cd4e 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1247,6 +1247,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>  		struct strbuf out = STRBUF_INIT;
>  		struct strbuf err = STRBUF_INIT;
> +		struct pretty_print_describe_status *describe_status;
> +
> +		describe_status = c->pretty_ctx->describe_status;
> +		if (describe_status) {
> +			if (!describe_status->max_invocations)
> +				return 0;
> +			describe_status->max_invocations--;
> +		}
>
>  		cmd.git_cmd = 1;
>  		strvec_push(&cmd.args, "describe");
> diff --git a/pretty.h b/pretty.h
> index 7ce6c0b437..27b15947ff 100644
> --- a/pretty.h
> +++ b/pretty.h
> @@ -23,6 +23,10 @@ enum cmit_fmt {
>  	CMIT_FMT_UNSPECIFIED
>  };
>
> +struct pretty_print_describe_status {
> +	unsigned int max_invocations;
> +};
> +
>  struct pretty_print_context {
>  	/*
>  	 * Callers should tweak these to change the behavior of pp_* functions.
> @@ -44,6 +48,7 @@ struct pretty_print_context {
>  	int color;
>  	struct ident_split *from_ident;
>  	unsigned encode_email_headers:1;
> +	struct pretty_print_describe_status *describe_status;
>
>  	/*
>  	 * Fields below here are manipulated internally by pp_* functions and
> diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
> index e9aa97117a..1c9ce3956b 100755
> --- a/t/t5001-archive-attr.sh
> +++ b/t/t5001-archive-attr.sh
> @@ -128,4 +128,18 @@ test_expect_success 'export-subst' '
>  	test_cmp substfile2 archive/substfile2
>  '
>
> +test_expect_success 'export-subst expands %(describe) once' '
> +	echo "\$Format:%(describe)\$" >substfile3 &&
> +	echo "\$Format:%(describe)\$" >>substfile3 &&
> +	echo "\$Format:%(describe)${LF}%(describe)\$" >substfile4 &&
> +	git add substfile[34] &&
> +	git commit -m export-subst-describe &&
> +	git tag -m export-subst-describe export-subst-describe &&
> +	git archive HEAD >archive-describe.tar &&
> +	extract_tar_to_dir archive-describe &&
> +	desc=$(git describe) &&
> +	grep -F "$desc" archive-describe/substfile[34] >substituted &&
> +	test_line_count = 1 substituted
> +'
> +
>  test_done

This whole thing seems rather backwards as a solution to the "we run it
N times" problem I mentioned in <87k0r7a4sr.fsf@evledraar.gmail.com>.

Instead of taking the trouble of putting a limit in the
pretty_print_context so we don't call it N times for the same commit,
why not just put the strbuf with the result in that same struct?

Then you can have it millions of times, and it won't be any more
expensive than the other existing %(format) specifiers (actually cheaper
than most).

Or is there some edge case I'm missing here where "git archive" can
either be fed N commits and we share the context, or we share the
context across formatting different revisions in some cases?

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Am 28.02.21 um 16:41 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sun, Feb 28 2021, René Scharfe. wrote:
>
>> Am 17.02.21 um 19:31 schrieb Jeff King:
>>> On Sun, Feb 14, 2021 at 11:10:57AM +0100, René Scharfe. wrote:
>>>
>>>> Allow restricting the tags used by the placeholder %(describe) with the
>>>> options match and exclude.  E.g. the following command describes the
>>>> current commit using official version tags, without those for release
>>>> candidates:
>>>>
>>>>    $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'
>>>
>>> An interesting side effect of this series is that it allows remote users
>>> asking for archives to fill in this data, too (by using export-subst
>>> placeholders). That includes servers allowing "git archive --remote",
>>> but also services like GitHub that will run git-archive on behalf of
>>> clients.
>>>
>>> I wonder what avenues for mischief this provides. Certainly using extra
>>> CPU to run git-describe.
>>
>> A repository can contain millions of files, each file can contain
>> millions of $Format:...$ sequences and each of them can contain millions
>> of %(describe) placeholders.  Each of them could have different match or
>> exclude args to prevent caching.  Allowing a single request to cause
>> trillions of calls of git describe sounds excessive.  Let's limit this.
>>
>> -- >8 --
>> Subject: [PATCH] archive: expand only a single %(describe) per archive
>>
>> Every %(describe) placeholder in $Format:...$ strings in files with the
>> attribute export-subst is expanded by calling git describe.  This can
>> potentially result in a lot of such calls per archive.  That's OK for
>> local repositories under control of the user of git archive, but could
>> be a problem for hosted repositories.
>>
>> Expand only a single %(describe) placeholder per archive for now to
>> avoid denial-of-service attacks.  We can make this limit configurable
>> later if needed, but let's start out simple.
>>
>> Reported-by: Jeff King <peff@peff.net>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  Documentation/gitattributes.txt |  3 ++-
>>  archive.c                       | 16 ++++++++++------
>>  archive.h                       |  2 ++
>>  pretty.c                        |  8 ++++++++
>>  pretty.h                        |  5 +++++
>>  t/t5001-archive-attr.sh         | 14 ++++++++++++++
>>  6 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index e84e104f93..0a60472bb5 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -1174,7 +1174,8 @@ tag then no replacement will be done.  The placeholders are the same
>>  as those for the option `--pretty=format:` of linkgit:git-log[1],
>>  except that they need to be wrapped like this: `$Format:PLACEHOLDERS$`
>>  in the file.  E.g. the string `$Format:%H$` will be replaced by the
>> -commit hash.
>> +commit hash.  However, only one `%(describe)` placeholder is expanded
>> +per archive to avoid denial-of-service attacks.
>>
>>
>>  Packing objects
>> diff --git a/archive.c b/archive.c
>> index 5919d9e505..2dd2236ae0 100644
>> --- a/archive.c
>> +++ b/archive.c
>> @@ -37,13 +37,10 @@ void init_archivers(void)
>>
>>  static void format_subst(const struct commit *commit,
>>  			 const char *src, size_t len,
>> -			 struct strbuf *buf)
>> +			 struct strbuf *buf, struct pretty_print_context *ctx)
>>  {
>>  	char *to_free = NULL;
>>  	struct strbuf fmt = STRBUF_INIT;
>> -	struct pretty_print_context ctx = {0};
>> -	ctx.date_mode.type = DATE_NORMAL;
>> -	ctx.abbrev = DEFAULT_ABBREV;
>>
>>  	if (src == buf->buf)
>>  		to_free = strbuf_detach(buf, NULL);
>> @@ -61,7 +58,7 @@ static void format_subst(const struct commit *commit,
>>  		strbuf_add(&fmt, b + 8, c - b - 8);
>>
>>  		strbuf_add(buf, src, b - src);
>> -		format_commit_message(commit, fmt.buf, buf, &ctx);
>> +		format_commit_message(commit, fmt.buf, buf, ctx);
>>  		len -= c + 1 - src;
>>  		src  = c + 1;
>>  	}
>> @@ -94,7 +91,7 @@ static void *object_file_to_archive(const struct archiver_args *args,
>>  		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
>>  		convert_to_working_tree(args->repo->index, path, buf.buf, buf.len, &buf, &meta);
>>  		if (commit)
>> -			format_subst(commit, buf.buf, buf.len, &buf);
>> +			format_subst(commit, buf.buf, buf.len, &buf, args->pretty_ctx);
>>  		buffer = strbuf_detach(&buf, &size);
>>  		*sizep = size;
>>  	}
>> @@ -633,12 +630,19 @@ int write_archive(int argc, const char **argv, const char *prefix,
>>  		  const char *name_hint, int remote)
>>  {
>>  	const struct archiver *ar = NULL;
>> +	struct pretty_print_describe_status describe_status = {0};
>> +	struct pretty_print_context ctx = {0};
>>  	struct archiver_args args;
>>  	int rc;
>>
>>  	git_config_get_bool("uploadarchive.allowunreachable", &remote_allow_unreachable);
>>  	git_config(git_default_config, NULL);
>>
>> +	describe_status.max_invocations = 1;
>> +	ctx.date_mode.type = DATE_NORMAL;
>> +	ctx.abbrev = DEFAULT_ABBREV;
>> +	ctx.describe_status = &describe_status;
>> +	args.pretty_ctx = &ctx;
>>  	args.repo = repo;
>>  	args.prefix = prefix;
>>  	string_list_init(&args.extra_files, 1);
>> diff --git a/archive.h b/archive.h
>> index 33551b7ee1..49fab71aaf 100644
>> --- a/archive.h
>> +++ b/archive.h
>> @@ -5,6 +5,7 @@
>>  #include "pathspec.h"
>>
>>  struct repository;
>> +struct pretty_print_context;
>>
>>  struct archiver_args {
>>  	struct repository *repo;
>> @@ -22,6 +23,7 @@ struct archiver_args {
>>  	unsigned int convert : 1;
>>  	int compression_level;
>>  	struct string_list extra_files;
>> +	struct pretty_print_context *pretty_ctx;
>>  };
>>
>>  /* main api */
>> diff --git a/pretty.c b/pretty.c
>> index c612d2ac9b..032e89cd4e 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1247,6 +1247,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>>  		struct child_process cmd = CHILD_PROCESS_INIT;
>>  		struct strbuf out = STRBUF_INIT;
>>  		struct strbuf err = STRBUF_INIT;
>> +		struct pretty_print_describe_status *describe_status;
>> +
>> +		describe_status = c->pretty_ctx->describe_status;
>> +		if (describe_status) {
>> +			if (!describe_status->max_invocations)
>> +				return 0;
>> +			describe_status->max_invocations--;
>> +		}
>>
>>  		cmd.git_cmd = 1;
>>  		strvec_push(&cmd.args, "describe");
>> diff --git a/pretty.h b/pretty.h
>> index 7ce6c0b437..27b15947ff 100644
>> --- a/pretty.h
>> +++ b/pretty.h
>> @@ -23,6 +23,10 @@ enum cmit_fmt {
>>  	CMIT_FMT_UNSPECIFIED
>>  };
>>
>> +struct pretty_print_describe_status {
>> +	unsigned int max_invocations;
>> +};
>> +
>>  struct pretty_print_context {
>>  	/*
>>  	 * Callers should tweak these to change the behavior of pp_* functions.
>> @@ -44,6 +48,7 @@ struct pretty_print_context {
>>  	int color;
>>  	struct ident_split *from_ident;
>>  	unsigned encode_email_headers:1;
>> +	struct pretty_print_describe_status *describe_status;
>>
>>  	/*
>>  	 * Fields below here are manipulated internally by pp_* functions and
>> diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
>> index e9aa97117a..1c9ce3956b 100755
>> --- a/t/t5001-archive-attr.sh
>> +++ b/t/t5001-archive-attr.sh
>> @@ -128,4 +128,18 @@ test_expect_success 'export-subst' '
>>  	test_cmp substfile2 archive/substfile2
>>  '
>>
>> +test_expect_success 'export-subst expands %(describe) once' '
>> +	echo "\$Format:%(describe)\$" >substfile3 &&
>> +	echo "\$Format:%(describe)\$" >>substfile3 &&
>> +	echo "\$Format:%(describe)${LF}%(describe)\$" >substfile4 &&
>> +	git add substfile[34] &&
>> +	git commit -m export-subst-describe &&
>> +	git tag -m export-subst-describe export-subst-describe &&
>> +	git archive HEAD >archive-describe.tar &&
>> +	extract_tar_to_dir archive-describe &&
>> +	desc=$(git describe) &&
>> +	grep -F "$desc" archive-describe/substfile[34] >substituted &&
>> +	test_line_count = 1 substituted
>> +'
>> +
>>  test_done
>
> This whole thing seems rather backwards as a solution to the "we run it
> N times" problem I mentioned in <87k0r7a4sr.fsf@evledraar.gmail.com>.

In the referenced message you pointed out that using %(describe) more
than once incurs the cost of a call to "git describe" each time and
asked for a way to avoid that.

This patch here prevents the second and later calls for "git archive".
It's not intended to speed up expanding repeated placeholders, but
rather to reject maliciously crafted placeholders even if they are not
the same.

So does that mean you have the requirement to use %(describe) more
than once in the same archive, and this DoS protection would be too
strict for you?  In that case we'd need a way to increase the limit,
e.g. a configuration variable or command line option.  Otherwise I
don't see the connection between your message and this patch.

Side note: Keeping the "git describe" running and feeding it commits
one by one through a pipe (with a new --stdin option, I suppose) as
mentioned in <66720982-04d6-3096-9ea2-ea5bc3fcd121@web.de> would
speed up expanding repeated placeholders as well.

> Instead of taking the trouble of putting a limit in the
> pretty_print_context so we don't call it N times for the same commit,
> why not just put the strbuf with the result in that same struct?
>
> Then you can have it millions of times, and it won't be any more
> expensive than the other existing %(format) specifiers (actually cheaper
> than most).

Each %(describe) placeholder can have a unique set of match or exclude
arguments.  Caching them all would increase the strength of a DoS
attack.  Caching a few of them would be OK, but is ineffective in
reducing the strength of the attack.

> Or is there some edge case I'm missing here where "git archive" can
> either be fed N commits and we share the context, or we share the
> context across formatting different revisions in some cases?

git archive currently works only on a single commit.

René

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Am 02.03.21 um 17:00 schrieb René Scharfe.:
> Am 28.02.21 um 16:41 schrieb Ævar Arnfjörð Bjarmason:
>> Instead of taking the trouble of putting a limit in the
>> pretty_print_context so we don't call it N times for the same commit,
>> why not just put the strbuf with the result in that same struct?
>>
>> Then you can have it millions of times, and it won't be any more
>> expensive than the other existing %(format) specifiers (actually cheaper
>> than most).
>
> Each %(describe) placeholder can have a unique set of match or exclude
> arguments.  Caching them all would increase the strength of a DoS
> attack.  Caching a few of them would be OK, but is ineffective in
> reducing the strength of the attack.

The script at the bottom creates archives that illustrate the issue. On
a repo generated with parameter 10 (10 files with 10 $Format:...$ with
10 %(describe) placeholders, so 1000 total), I get the following number
with v2.30.1, which ignores %(describe):

Benchmark #1: git archive HEAD
  Time (mean ± σ):       2.2 ms ±   0.2 ms    [User: 0.9 ms, System: 1.0 ms]
  Range (min … max):     1.8 ms …   2.8 ms    705 runs

  Warning: Command took less than 5 ms to complete. Results might be inaccurate.

The version in next expands all placeholders and takes three orders of
magnitude longer:

Benchmark #1: git archive HEAD
  Time (mean ± σ):      2.300 s ±  0.003 s    [User: 819.0 ms, System: 1200.0 ms]
  Range (min … max):    2.293 s …  2.305 s    10 runs

The proposed patch to expand only a single placeholder gets the runtime
back under control:

Benchmark #1: git archive HEAD
  Time (mean ± σ):       4.7 ms ±   0.3 ms    [User: 1.8 ms, System: 2.2 ms]
  Range (min … max):     4.2 ms …   7.0 ms    451 runs

  Warning: Command took less than 5 ms to complete. Results might be inaccurate.

Using parameter 100 takes about a second to create the repo, but the git
archive version in next already needs longer to tar it up than I'm
willing to wait.


#!/bin/sh
n=$1
mkdir $n
cd $n
git init
for i in $(seq $n)
do
	awk -v i=$i -v n=$n 'END {
		for (j = 0; j < n; j++) {
			print "$Format:"
			for (k = 0; k < n; k++) {
				print "%(describe:exclude=x-" i "-" j "-" k ")"
			}
			print "$"
		}
	}' </dev/null >"file$i"
done
git add file*
echo "file* export-subst" >.gitattributes
git add .gitattributes
git commit -m initial
for tagno in $(seq $n)
do
	git tag -m "$tagno" "tag$tagno"
done

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Am 01.03.21 um 18:54 schrieb Junio C Hamano:
> René Scharfe. <l.s.r@web.de> writes:
>
>> Am 17.02.21 um 19:31 schrieb Jeff King:
>>> On Sun, Feb 14, 2021 at 11:10:57AM +0100, René Scharfe. wrote:
>>>
>>>> Allow restricting the tags used by the placeholder %(describe) with the
>>>> options match and exclude.  E.g. the following command describes the
>>>> current commit using official version tags, without those for release
>>>> candidates:
>>>>
>>>>    $ git log -1 --format='%(describe:match=v[0-9]*,exclude=*rc*)'
>>>
>>> An interesting side effect of this series is that it allows remote users
>>> asking for archives to fill in this data, too (by using export-subst
>>> placeholders). That includes servers allowing "git archive --remote",
>>> but also services like GitHub that will run git-archive on behalf of
>>> clients.
>>>
>>> I wonder what avenues for mischief this provides. Certainly using extra
>>> CPU to run git-describe.
>>
>> A repository can contain millions of files, each file can contain
>> millions of $Format:...$ sequences and each of them can contain millions
>> of %(describe) placeholders.  Each of them could have different match or
>> exclude args to prevent caching.  Allowing a single request to cause
>> trillions of calls of git describe sounds excessive.  Let's limit this.
>
> An invocation of "git archive" would have to deal with a single
> commit, no?  I wonder if it is a more fruitful direction to go to
> teach format_subst() to "cache" the mapping from <placeholders> to
> <resulting-string> and reuse.

Yes, git archive only works on a single commit.  Caching cannot help
against a DoS attack using describe placeholders with different match
or exclude arguments.

René

Re: [PATCH 2/2] pretty: add merge and exclude options to %(describe)

[René Scharfe.]

Am 17.02.21 um 19:38 schrieb Junio C Hamano:
> I wonder, in addition to "match" and "exclude", if we want to allow
> "always" as well.

I added "match" because that describe option is used by GIT-VERSION-GEN,
so I imagine it's generally useful for version names in spec files.  Not
sure why I misspelled it "merge" in the subject, though. o_O

"exclude" is not in there, but I threw it in anyway, as the example in
77d21f29ea (describe: teach describe negative pattern matches,
2017-01-18) made sense to me.  It complements "match" nicely.

I had "always" in proof-of-concept patch because I hadn't decided what
to do with commits that git describe doesn't find a description for,
and wanted to check the full output of git log --pretty='%(describe)'.
For a release tarball of a repo that lacks tags it would be easier to
use %h instead of %(describe:always) -- or tag the release.

That's why I didn't include "always" in the latest patches, but if it
turns out to be useful for someone then I wouldn't them adding it.

> Also, looking further into the future, I wonder if we should aim to
> eventually unify %h and %H as well as %(describe) into one,
> i.e. various ways to spell a commit object name, given that there is
> a separate effort underway to unify pretty and for-each-ref format
> strings.
>
> E.g. %(objectname) is the same as %H, and %(objectname:short) is the
> same as %h, so this might be %(objectname:describe), or something
> along the line.

According to the glossary and object name is:

        The unique identifier of an object.  The
        object name is usually represented by a 40 character
        hexadecimal string.  Also colloquially called SHA-1.

And that's how I understand it as well.  The object layer with it's
hashes on one hand and descriptions based on refs and commit relations
on the other are separate things in my mind.  %(describe) falling back
to %h when :always is given makes sense to me; %(objectname) "falling
forward" to show describe output feels like a layering violation.

René

Re: [PATCH 1/2] pretty: add %(describe)

[Eli Schwartz]

[-- Attachment #1.1: Type: text/plain, Size: 3488 bytes --]

On 2/14/21 5:04 AM, René Scharfe wrote:
> Add a format placeholder for describe output.  Implement it by actually
> calling git describe, which is simple and guarantees correctness.  It's
> intended to be used with $Format:...$ in files with the attribute
> export-subst and git archive.  It can also be used with git log etc.,
> even though that's going to be slow due to the fork for each commit.

This patch works great for me.

In fact, it even works fast enough for git log to not noticeably slow
down unless I really stomp on the "Page Down" button. At least on Linux...

Thanks for working on this!

> Suggested-by: Eli Schwartz <eschwartz@archlinux.org>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/pretty-formats.txt |  2 ++
>  pretty.c                         | 17 +++++++++++++++++
>  t/t4205-log-pretty-formats.sh    | 10 ++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 6b59e28d44..bb8c05bc21 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -208,6 +208,8 @@ The placeholders are:
>  '%cs':: committer date, short format (`YYYY-MM-DD`)
>  '%d':: ref names, like the --decorate option of linkgit:git-log[1]
>  '%D':: ref names without the " (", ")" wrapping.
> +'%(describe)':: human-readable name, like linkgit:git-describe[1];
> +		empty string for undescribable commits
>  '%S':: ref name given on the command line by which the commit was reached
>         (like `git log --source`), only works with `git log`
>  '%e':: encoding
> diff --git a/pretty.c b/pretty.c
> index b4ff3f602f..a0c427fb61 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -12,6 +12,7 @@
>  #include "reflog-walk.h"
>  #include "gpg-interface.h"
>  #include "trailer.h"
> +#include "run-command.h"
> 
>  static char *user_format;
>  static struct cmt_fmt_map {
> @@ -1214,6 +1215,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  		return parse_padding_placeholder(placeholder, c);
>  	}
> 
> +	if (skip_prefix(placeholder, "(describe)", &arg)) {
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +		struct strbuf out = STRBUF_INIT;
> +		struct strbuf err = STRBUF_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_push(&cmd.args, "describe");
> +		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
> +		pipe_command(&cmd, NULL, 0, &out, 0, &err, 0);
> +		strbuf_rtrim(&out);
> +		strbuf_addbuf(sb, &out);
> +		strbuf_release(&out);
> +		strbuf_release(&err);
> +		return arg - placeholder;
> +	}
> +
>  	/* these depend on the commit */
>  	if (!commit->object.parsed)
>  		parse_object(the_repository, &commit->object.oid);
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 749bc1431a..5a44fa447d 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -962,4 +962,14 @@ test_expect_success 'log --pretty=reference is colored appropriately' '
>  	test_cmp expect actual
>  '
> 
> +test_expect_success '%(describe) vs git describe' '
> +	git log --format="%H" | while read hash
> +	do
> +		echo "$hash $(git describe $hash)"
> +	done >expect &&
> +	git log --format="%H %(describe)" >actual 2>err &&
> +	test_cmp expect actual &&
> +	test_must_be_empty err
> +'
> +
>  test_done
> --
> 2.30.1
> 


-- 
Eli Schwartz
Bug Wrangler and Trusted User


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pretty: add %(describe)

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 14 2021, René Scharfe wrote:

> Add a format placeholder for describe output.  Implement it by actually
> calling git describe, which is simple and guarantees correctness.  It's
> intended to be used with $Format:...$ in files with the attribute
> export-subst and git archive. 

Does it really guarantee correctness though? In "builtin/describe.c" we
first walk over the refs and use that to format all N items we're asked
about.

Under "git log" this is presumably in a race where refs added/deleted
during the run of "git log" will change the describe output to be
inconsistent with earlier lines.

> It can also be used with git log etc., even though that's going to be
> slow due to the fork for each commit.

Re: [PATCH 1/2] pretty: add %(describe)

[René Scharfe.]

Am 16.02.21 um 14:00 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sun, Feb 14 2021, René Scharfe wrote:
>
>> Add a format placeholder for describe output.  Implement it by actually
>> calling git describe, which is simple and guarantees correctness.  It's
>> intended to be used with $Format:...$ in files with the attribute
>> export-subst and git archive.
>
> Does it really guarantee correctness though? In "builtin/describe.c" we
> first walk over the refs and use that to format all N items we're asked
> about.
>
> Under "git log" this is presumably in a race where refs added/deleted
> during the run of "git log" will change the describe output to be
> inconsistent with earlier lines.

Right, didn't think of that aspect.  So we'd better warn about the
raciness in the documentation.

We could improve that by keeping a single describe process around and
feeding it object names through a pipe as we go.  The results would
still become outdated after a ref is added or removed, but they'd be
consistent.  This would be faster as well.

René

Re: [PATCH 1/2] pretty: add %(describe)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Sun, Feb 14 2021, René Scharfe wrote:
>
>> Add a format placeholder for describe output.  Implement it by actually
>> calling git describe, which is simple and guarantees correctness.  It's
>> intended to be used with $Format:...$ in files with the attribute
>> export-subst and git archive. 
>
> Does it really guarantee correctness though? In "builtin/describe.c" we
> first walk over the refs and use that to format all N items we're asked
> about.
>
> Under "git log" this is presumably in a race where refs added/deleted
> during the run of "git log" will change the describe output to be
> inconsistent with earlier lines.

Yes, but it is not a news that the describe for a given commit will
not stay the constant while you add more objects to the repository
or you change the tags, whether the "describe" is driven internally
by "git log" or by the end-user, so I am not sure how that becomes
an issue.  If the output is inconsistent in a quiescent repository,
that would be a problem, though.

Puzzled.

Thanks.

Re: [PATCH 1/2] pretty: add %(describe)

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 16 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Sun, Feb 14 2021, René Scharfe wrote:
>>
>>> Add a format placeholder for describe output.  Implement it by actually
>>> calling git describe, which is simple and guarantees correctness.  It's
>>> intended to be used with $Format:...$ in files with the attribute
>>> export-subst and git archive. 
>>
>> Does it really guarantee correctness though? In "builtin/describe.c" we
>> first walk over the refs and use that to format all N items we're asked
>> about.
>>
>> Under "git log" this is presumably in a race where refs added/deleted
>> during the run of "git log" will change the describe output to be
>> inconsistent with earlier lines.
>
> Yes, but it is not a news that the describe for a given commit will
> not stay the constant while you add more objects to the repository
> or you change the tags, whether the "describe" is driven internally
> by "git log" or by the end-user, so I am not sure how that becomes
> an issue.  If the output is inconsistent in a quiescent repository,
> that would be a problem, though.
>
> Puzzled.

Usually something shelling out has going for it is that even if it's
slow it's at least known to be bug-free, after all we use the command
like that already.

I just wanted to point out this edge case, for "git describe <commits>"
we do the ref list once at the beginning so our N list will be
consistent within itself.

Whereas one might expect "git log" to also format such a list, but due
to the internal implementation the semantics are different.

Re: [PATCH 1/2] pretty: add %(describe)

[René Scharfe.]

Am 17.02.21 um 01:47 schrieb Ævar Arnfjörð Bjarmason:
> Usually something shelling out has going for it is that even if it's
> slow it's at least known to be bug-free, after all we use the command
> like that already.
>
> I just wanted to point out this edge case, for "git describe <commits>"
> we do the ref list once at the beginning so our N list will be
> consistent within itself.
>
> Whereas one might expect "git log" to also format such a list, but due
> to the internal implementation the semantics are different.

It shouldn't matter for the intended use case (git archive containing a
spec file with a single "$Format:%(describe)$") and I cannot imagine how
consistency in the face of tag changes would be useful (what application
would be OK with consistently outdated output, but break with partly
outdated descriptions).  But it makes sense to mention it in the docs.

-- >8 --
Subject: [PATCH] pretty: document multiple %(describe) being inconsistent

Each %(describe) placeholder is expanded using a separate git describe
call.  Their outputs depend on the tags present at the time, so there's
no consistency guarantee.  Document that fact.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/pretty-formats.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 231010e6ef..45133066e4 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -212,7 +212,9 @@ The placeholders are:
 			  linkgit:git-describe[1]; empty string for
 			  undescribable commits.  The `describe` string
 			  may be followed by a colon and zero or more
-			  comma-separated options.
+			  comma-separated options.  Descriptions can be
+			  inconsistent when tags are added or removed at
+			  the same time.
 +
 ** 'match=<pattern>': Only consider tags matching the given
    `glob(7)` pattern, excluding the "refs/tags/" prefix.
--
2.30.1

Re: [PATCH 1/2] pretty: add %(describe)

[René Scharfe.]

Am 01.03.21 um 18:50 schrieb Junio C Hamano:
> René Scharfe. <l.s.r@web.de> writes:
>
>> Am 17.02.21 um 01:47 schrieb Ævar Arnfjörð Bjarmason:
>>> Usually something shelling out has going for it is that even if it's
>>> slow it's at least known to be bug-free, after all we use the command
>>> like that already.
>>>
>>> I just wanted to point out this edge case, for "git describe <commits>"
>>> we do the ref list once at the beginning so our N list will be
>>> consistent within itself.
>>>
>>> Whereas one might expect "git log" to also format such a list, but due
>>> to the internal implementation the semantics are different.
>>
>> It shouldn't matter for the intended use case (git archive containing a
>> spec file with a single "$Format:%(describe)$") and I cannot imagine how
>> consistency in the face of tag changes would be useful (what application
>> would be OK with consistently outdated output, but break with partly
>> outdated descriptions).  But it makes sense to mention it in the docs.
>
> Does it?
>
> As long as the reader understands "git describe" is about measuring
> the "location" of given commits relative to the annotated tags (or
> whatever anchor points the invocation chooses to use), I would say
> that it is unlikely that the reader does not to realize the
> ramifications of changing tags in the middle.

I actually agree, but I'm biased.  The thing is: The question came up
and needed answering, so there is a chance that it might help someone
else as well.

> While it may not be incorrect per-se (hence it may "not hurt"),
> making the description longer does hurt the readers' experience.

The added sentence would look better in a smaller font, or in a
footnote. :-|

> So, I am a bit skeptical if this is a good change, but will queue
> for now anyway.
>
> Thanks.
>
>
>
>> -- >8 --
>> Subject: [PATCH] pretty: document multiple %(describe) being inconsistent
>>
>> Each %(describe) placeholder is expanded using a separate git describe
>> call.  Their outputs depend on the tags present at the time, so there's
>> no consistency guarantee.  Document that fact.
>>
>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  Documentation/pretty-formats.txt | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index 231010e6ef..45133066e4 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -212,7 +212,9 @@ The placeholders are:
>>  			  linkgit:git-describe[1]; empty string for
>>  			  undescribable commits.  The `describe` string
>>  			  may be followed by a colon and zero or more
>> -			  comma-separated options.
>> +			  comma-separated options.  Descriptions can be
>> +			  inconsistent when tags are added or removed at
>> +			  the same time.
>>  +
>>  ** 'match=<pattern>': Only consider tags matching the given
>>     `glob(7)` pattern, excluding the "refs/tags/" prefix.
>> --
>> 2.30.1

Re: [PATCH 1/2] pretty: add %(describe)

[Ævar Arnfjörð Bjarmason]

On Sun, Feb 14 2021, René Scharfe wrote:

> +'%(describe)':: human-readable name, like linkgit:git-describe[1];
> +		empty string for undescribable commits

In the case of undescribable we've got the subcommand exiting non-zero
and we ignore it. The right thing in this case given how the rest of
format arguments work, but maybe something to explicitly test for?
>
> +	if (skip_prefix(placeholder, "(describe)", &arg)) {
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +		struct strbuf out = STRBUF_INIT;
> +		struct strbuf err = STRBUF_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_push(&cmd.args, "describe");
> +		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
> +		pipe_command(&cmd, NULL, 0, &out, 0, &err, 0);
> +		strbuf_rtrim(&out);
> +		strbuf_addbuf(sb, &out);
> +		strbuf_release(&out);
> +		strbuf_release(&err);
> +		return arg - placeholder;
> +	}

There's another edge case in this: if you do "%(describe)%(describe)"
it'll be run twice for the rev, 3 times if you add another "%(describe)"
etc. I don't know if pretty.c has an easy way to cache/avoid that.

Re: [PATCH 1/2] pretty: add %(describe)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Sun, Feb 14 2021, René Scharfe wrote:
>
>> +'%(describe)':: human-readable name, like linkgit:git-describe[1];
>> +		empty string for undescribable commits
>
> In the case of undescribable we've got the subcommand exiting non-zero
> and we ignore it. The right thing in this case given how the rest of
> format arguments work, but maybe something to explicitly test for?
>>
>> +	if (skip_prefix(placeholder, "(describe)", &arg)) {
>> +		struct child_process cmd = CHILD_PROCESS_INIT;
>> +		struct strbuf out = STRBUF_INIT;
>> +		struct strbuf err = STRBUF_INIT;
>> +
>> +		cmd.git_cmd = 1;
>> +		strvec_push(&cmd.args, "describe");
>> +		strvec_push(&cmd.args, oid_to_hex(&commit->object.oid));
>> +		pipe_command(&cmd, NULL, 0, &out, 0, &err, 0);
>> +		strbuf_rtrim(&out);
>> +		strbuf_addbuf(sb, &out);
>> +		strbuf_release(&out);
>> +		strbuf_release(&err);
>> +		return arg - placeholder;
>> +	}
>
> There's another edge case in this: if you do "%(describe)%(describe)"
> it'll be run twice for the rev, 3 times if you add another "%(describe)"
> etc. I don't know if pretty.c has an easy way to cache/avoid that.

Just like for-each-ref that has the "atom" system to lazily parse
and cache a computed result so that multiple invocations of the same
placeholder will have to prepare a string only once, the pretty side
has the "format_commit_context" structure that can be used to cache
values that are expensive to compute in case it is used more than
once, so we could use it.

I however suspect that we may already be leaking some cacheed values
in the current code.  For example, there is "signature_check"
instance that is used to handle %G and we do use it to call
check_signature(), but a call to signature_check_clear() to release
resources is nowhere to be seen as far as I can tell.

Re: [PATCH 1/2] pretty: add %(describe)

[René Scharfe.]

Am 17.02.21 um 01:58 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sun, Feb 14 2021, René Scharfe wrote:
>
>> +'%(describe)':: human-readable name, like linkgit:git-describe[1];
>> +		empty string for undescribable commits
>
> In the case of undescribable we've got the subcommand exiting non-zero
> and we ignore it. The right thing in this case given how the rest of
> format arguments work, but maybe something to explicitly test for?

The test convers it, but we can surely make that easier to see.

-- >8 --
Subject: [PATCH] t4205: assert %(describe) test coverage

Document that the test is covering both describable and
undescribable commits.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t4205-log-pretty-formats.sh | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index b47a0bd9eb..cabdf7d57a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -965,8 +965,17 @@ test_expect_success 'log --pretty=reference is colored appropriately' '
 test_expect_success '%(describe) vs git describe' '
 	git log --format="%H" | while read hash
 	do
-		echo "$hash $(git describe $hash)"
+		if desc=$(git describe $hash)
+		then
+			: >expect-contains-good
+		else
+			: >expect-contains-bad
+		fi &&
+		echo "$hash $desc"
 	done >expect &&
+	test_path_exists expect-contains-good &&
+	test_path_exists expect-contains-bad &&
+
 	git log --format="%H %(describe)" >actual 2>err &&
 	test_cmp expect actual &&
 	test_must_be_empty err
--
2.30.1

Re: [PATCH 1/2] pretty: add %(describe)

[René Scharfe.]

Am 01.03.21 um 18:45 schrieb Junio C Hamano:
> René Scharfe. <l.s.r@web.de> writes:
>
>> Subject: [PATCH] t4205: assert %(describe) test coverage
>>
>> Document that the test is covering both describable and
>> undescribable commits.
>>
>> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  t/t4205-log-pretty-formats.sh | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> index b47a0bd9eb..cabdf7d57a 100755
>> --- a/t/t4205-log-pretty-formats.sh
>> +++ b/t/t4205-log-pretty-formats.sh
>> @@ -965,8 +965,17 @@ test_expect_success 'log --pretty=reference is colored appropriately' '
>>  test_expect_success '%(describe) vs git describe' '
>>  	git log --format="%H" | while read hash
>>  	do
>> -		echo "$hash $(git describe $hash)"
>> +		if desc=$(git describe $hash)
>> +		then
>> +			: >expect-contains-good
>> +		else
>> +			: >expect-contains-bad
>> +		fi &&
>> +		echo "$hash $desc"
>>  	done >expect &&
>> +	test_path_exists expect-contains-good &&
>> +	test_path_exists expect-contains-bad &&
>
> Hmph, I am not sure why we want temporary files for this (and I
> doubt this "documenting" adds that much value to the tests to begin
> with), but OK.  Will queue.

Variables would suffice, but make debugging harder.  test_path_exists
will at least print a suggestive file name.  Perhaps we should add a
test_assert?

The added checks guard against neutering the test accidentally e.g.
by tagging the currently undescribable commit in the setup phase.
That would be hard to detect without it.

>
>>  	git log --format="%H %(describe)" >actual 2>err &&
>>  	test_cmp expect actual &&
>>  	test_must_be_empty err
>> --
>> 2.30.1