gitattributes export-subst and software versioning

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

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

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.

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User

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

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 <redacted>
Signed-off-by: René Scharfe <redacted>
---
 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

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 <redacted>
---
 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

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!

-- 
Eli Schwartz
Bug Wrangler and Trusted User

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.

Am 16.02.21 um 14:00 schrieb Ævar Arnfjörð Bjarmason:

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

quoted

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é

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.

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

Am 17.02.21 um 01:58 schrieb Ævar Arnfjörð Bjarmason:

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

quoted

+'%(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 <redacted>
Signed-off-by: René Scharfe <redacted>
---
 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

Am 17.02.21 um 19:31 schrieb Jeff King:

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

quoted

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 <redacted>
Signed-off-by: René Scharfe <redacted>
---
 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

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

This whole thing seems rather backwards as a solution to the "we run it
N times" problem I mentioned in [off-list ref].

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?

Am 28.02.21 um 16:41 schrieb Ævar Arnfjörð Bjarmason:

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

quoted

Am 17.02.21 um 19:31 schrieb Jeff King:

quoted

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

quoted

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 <redacted>
Signed-off-by: René Scharfe <redacted>
---
 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 [off-list ref].

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 [off-list ref] 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é

Am 02.03.21 um 17:00 schrieb René Scharfe.:

Am 28.02.21 um 16:41 schrieb Ævar Arnfjörð Bjarmason:

quoted

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