[PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[René Scharfe]

Add support for a new attribute, specfile.  Files marked as being
specfiles are expanded by git-archive when they are written to an
archive.  It has no effect on worktree files.  The same placeholders
as those for the option --pretty=format: of git-log et al. can be
used.

The attribute is useful for creating auto-updating specfiles.  It is
limited by the underlying function format_commit_message(), though.
E.g. currently there is no placeholder for git-describe like output,
and expanded specfiles can't contain NUL bytes.  That can be fixed
in format_commit_message() later and will then benefit users of
git-log, too.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/gitattributes.txt |   14 ++++++++++
 archive-tar.c                   |    5 +++-
 archive-zip.c                   |    5 +++-
 archive.h                       |    3 ++
 builtin-archive.c               |   55 ++++++++++++++++++++++++++++++++++++++-
 t/t5000-tar-tree.sh             |   19 +++++++++++++
 6 files changed, 98 insertions(+), 3 deletions(-)

This should already be sufficient for the use case which Michael and
Thomas described a while ago, viz. adding a commit ID file to
generated archives.

Why did it take me that long to come up with such a simple patch?
There was a vacation and a feature freeze in between, but above all
I was only recently able to convince myself (using ugly code) that
format_commit_message() can indeed be made to expand placeholders
to git-describe strings..

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 46f9d59..47a621b 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -421,6 +421,20 @@ frotz	unspecified
 ----------------------------------------------------------------
 
 
+Creating an archive
+~~~~~~~~~~~~~~~~~~~
+
+`specfile`
+^^^^^^^^^^
+
+If the attribute `specfile` is set for a file then git will expand
+several placeholders when adding this file to an archive.  The
+expansion depends on the availability of a commit ID, i.e. if
+gitlink:git-archive[1] has been given a tree instead of a commit or a
+tag then no replacement will be done.  The placeholders are the same
+as those for the option `--pretty=format:` of gitlink:git-log[1].
+
+
 GIT
 ---
 Part of the gitlink:git[7] suite
diff --git a/archive-tar.c b/archive-tar.c
index 66fe3e3..c0d95da 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -17,6 +17,7 @@ static unsigned long offset;
 static time_t archive_time;
 static int tar_umask = 002;
 static int verbose;
+static const struct commit *commit;
 
 /* writes out the whole block, but only if it is full */
 static void write_if_needed(void)
@@ -285,7 +286,8 @@ static int write_tar_entry(const unsigned char *sha1,
 		buffer = NULL;
 		size = 0;
 	} else {
-		buffer = convert_sha1_file(path.buf, sha1, mode, &type, &size);
+		buffer = sha1_file_to_archive(path.buf, sha1, mode, &type,
+		                              &size, commit);
 		if (!buffer)
 			die("cannot read %s", sha1_to_hex(sha1));
 	}
@@ -304,6 +306,7 @@ int write_tar_archive(struct archiver_args *args)
 
 	archive_time = args->time;
 	verbose = args->verbose;
+	commit = args->commit;
 
 	if (args->commit_sha1)
 		write_global_extended_header(args->commit_sha1);
diff --git a/archive-zip.c b/archive-zip.c
index 444e162..f63dff3 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -12,6 +12,7 @@
 static int verbose;
 static int zip_date;
 static int zip_time;
+static const struct commit *commit;
 
 static unsigned char *zip_dir;
 static unsigned int zip_dir_size;
@@ -195,7 +196,8 @@ static int write_zip_entry(const unsigned char *sha1,
 		if (S_ISREG(mode) && zlib_compression_level != 0)
 			method = 8;
 		result = 0;
-		buffer = convert_sha1_file(path, sha1, mode, &type, &size);
+		buffer = sha1_file_to_archive(path, sha1, mode, &type, &size,
+		                              commit);
 		if (!buffer)
 			die("cannot read %s", sha1_to_hex(sha1));
 		crc = crc32(crc, buffer, size);
@@ -316,6 +318,7 @@ int write_zip_archive(struct archiver_args *args)
 	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
 	zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
 	verbose = args->verbose;
+	commit = args->commit;
 
 	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
 		char *base = xstrdup(args->base);
diff --git a/archive.h b/archive.h
index 6838dc7..5791e65 100644
--- a/archive.h
+++ b/archive.h
@@ -8,6 +8,7 @@ struct archiver_args {
 	const char *base;
 	struct tree *tree;
 	const unsigned char *commit_sha1;
+	const struct commit *commit;
 	time_t time;
 	const char **pathspec;
 	unsigned int verbose : 1;
@@ -42,4 +43,6 @@ extern int write_tar_archive(struct archiver_args *);
 extern int write_zip_archive(struct archiver_args *);
 extern void *parse_extra_zip_args(int argc, const char **argv);
 
+extern void *sha1_file_to_archive(const char *path, const unsigned char *sha1, unsigned int mode, enum object_type *type, unsigned long *size, const struct commit *commit);
+
 #endif	/* ARCHIVE_H */
diff --git a/builtin-archive.c b/builtin-archive.c
index 187491b..faccce3 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -10,6 +10,7 @@
 #include "exec_cmd.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "attr.h"
 
 static const char archive_usage[] = \
 "git-archive --format=<fmt> [--prefix=<prefix>/] [--verbose] [<extra>] <tree-ish> [path...]";
@@ -80,6 +81,57 @@ static int run_remote_archiver(const char *remote, int argc,
 	return !!rv;
 }
 
+static void *convert_to_archive(const char *path,
+                                const void *src, unsigned long *sizep,
+                                const struct commit *commit)
+{
+	static struct git_attr *attr_specfile;
+	struct git_attr_check check[1];
+	char *interpolated = NULL;
+	unsigned long allocated = 0;
+
+	if (!commit)
+		return NULL;
+
+        if (!attr_specfile)
+                attr_specfile = git_attr("specfile", 8);
+
+	check[0].attr = attr_specfile;
+	if (git_checkattr(path, ARRAY_SIZE(check), check))
+		return NULL;
+	if (!ATTR_TRUE(check[0].value))
+		return NULL;
+
+	*sizep = format_commit_message(commit, src, &interpolated, &allocated);
+
+	return interpolated;
+}
+
+void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
+                           unsigned int mode, enum object_type *type,
+                           unsigned long *size,
+                           const struct commit *commit)
+{
+	void *buffer, *converted;
+
+	buffer = read_sha1_file(sha1, type, size);
+	if (buffer && S_ISREG(mode)) {
+		converted = convert_to_working_tree(path, buffer, size);
+		if (converted) {
+			free(buffer);
+			buffer = converted;
+		}
+
+		converted = convert_to_archive(path, buffer, size, commit);
+		if (converted) {
+			free(buffer);
+			buffer = converted;
+		}
+	}
+
+	return buffer;
+}
+
 static int init_archiver(const char *name, struct archiver *ar)
 {
 	int rv = -1, i;
@@ -109,7 +161,7 @@ void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,
 	const unsigned char *commit_sha1;
 	time_t archive_time;
 	struct tree *tree;
-	struct commit *commit;
+	const struct commit *commit;
 	unsigned char sha1[20];
 
 	if (get_sha1(name, sha1))
@@ -142,6 +194,7 @@ void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,
 	}
 	ar_args->tree = tree;
 	ar_args->commit_sha1 = commit_sha1;
+	ar_args->commit = commit;
 	ar_args->time = archive_time;
 }
 
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 1a4c53a..3d5d01b 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -28,12 +28,15 @@ commit id embedding:
 TAR=${TAR:-tar}
 UNZIP=${UNZIP:-unzip}
 
+SPECFILEFORMAT=%H%n
+
 test_expect_success \
     'populate workdir' \
     'mkdir a b c &&
      echo simple textfile >a/a &&
      mkdir a/bin &&
      cp /bin/sh a/bin &&
+     printf "%s" "$SPECFILEFORMAT" >a/specfile &&
      ln -s a a/l1 &&
      (p=long_path_to_a_file && cd a &&
       for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -105,6 +108,22 @@ test_expect_success \
     'diff -r a c/prefix/a'
 
 test_expect_success \
+    'create an archive with a specfile' \
+    'echo specfile specfile >a/.gitattributes &&
+     git archive HEAD >f.tar &&
+     rm a/.gitattributes'
+
+test_expect_success \
+    'extract specfile' \
+    '(mkdir f && cd f && $TAR xf -) <f.tar'
+
+test_expect_success \
+     'validate specfile contents' \
+     'git log --max-count=1 "--pretty=format:$SPECFILEFORMAT" HEAD \
+      >f/a/specfile.expected &&
+      diff f/a/specfile.expected f/a/specfile'
+
+test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
 
-- 
1.5.3

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Johannes Schindelin]

Hi,

On Mon, 3 Sep 2007, Ren? Scharfe wrote:

> Add support for a new attribute, specfile.  Files marked as being
> specfiles are expanded by git-archive when they are written to an
> archive.  It has no effect on worktree files.  The same placeholders
> as those for the option --pretty=format: of git-log et al. can be
> used.

I almost like this approach.  Would it not be a little more useful if you 
could mark the placeholders with something like "$Format: xyz %c$"?  
Because then we could just shut up all those complainers that want to 
insert some revision specific information into the files, without 
affecting formats for printf().

Of course, the idea to keep the worktree unaffected is brilliant.

Ciao,
Dscho

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 3 Sep 2007, Ren? Scharfe wrote:
>
>> Add support for a new attribute, specfile.  Files marked as being
>> specfiles are expanded by git-archive when they are written to an
>> archive.  It has no effect on worktree files.  The same placeholders
>> as those for the option --pretty=format: of git-log et al. can be
>> used.
>
> I almost like this approach.  Would it not be a little more useful if you 
> could mark the placeholders with something like "$Format: xyz %c$"?  
> Because then we could just shut up all those complainers that want to 
> insert some revision specific information into the files, without 
> affecting formats for printf().
>
> Of course, the idea to keep the worktree unaffected is brilliant.

I think a bit more layering would be helpful: when using git-svn, one
would want to have things like $Id$ and $Date$ expanded, so maybe
attribute specs like

somefile: expandmarkers="$Date: %aD$ $Id: ....$"

would be nice having.  In the case of git-svn, I would expect them to
be generated from git-svn from the respective svn properties, so that
the user is not bothered with figuring out the awful $Id$ and whatever
strings.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> The attribute is useful for creating auto-updating specfiles.  It is
> limited by the underlying function format_commit_message(), though.
> E.g. currently there is no placeholder for git-describe like output,
> and expanded specfiles can't contain NUL bytes.  That can be fixed
> in format_commit_message() later and will then benefit users of
> git-log, too.

Interesting.  I however wonder if "specfile" is a good name for
this attribute, although I admit I do not think of anything
better offhand.

> Why did it take me that long to come up with such a simple patch?
> There was a vacation and a feature freeze in between, but above all
> I was only recently able to convince myself (using ugly code) that
> format_commit_message() can indeed be made to expand placeholders
> to git-describe strings..

Thanks.  Will take a look.

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Andreas Ericsson]

Junio C Hamano wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> The attribute is useful for creating auto-updating specfiles.  It is
>> limited by the underlying function format_commit_message(), though.
>> E.g. currently there is no placeholder for git-describe like output,
>> and expanded specfiles can't contain NUL bytes.  That can be fixed
>> in format_commit_message() later and will then benefit users of
>> git-log, too.
> 
> Interesting.  I however wonder if "specfile" is a good name for
> this attribute, although I admit I do not think of anything
> better offhand.
> 

"releasefile", perhaps?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Johannes Schindelin]

Hi,

On Tue, 4 Sep 2007, Andreas Ericsson wrote:

> Junio C Hamano wrote:
> > Ren? Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> > 
> > > The attribute is useful for creating auto-updating specfiles.  It is 
> > > limited by the underlying function format_commit_message(), though. 
> > > E.g. currently there is no placeholder for git-describe like output, 
> > > and expanded specfiles can't contain NUL bytes.  That can be fixed 
> > > in format_commit_message() later and will then benefit users of 
> > > git-log, too.
> > 
> > Interesting. I however wonder if "specfile" is a good name for this 
> > attribute, although I admit I do not think of anything better offhand.
> 
> "releasefile", perhaps?

Maybe we should not so much name it by purpose, but by function.  How 
about "substformat" for the attribute name, and replacing any 
$Format:blablub$ inside those files with something a la 
--pretty=format:blablub?

Ciao,
Dscho

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[René Scharfe]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 4 Sep 2007, Andreas Ericsson wrote:
> 
>> Junio C Hamano wrote:
>>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>>>
>>>> The attribute is useful for creating auto-updating specfiles.  It is 
>>>> limited by the underlying function format_commit_message(), though. 
>>>> E.g. currently there is no placeholder for git-describe like output, 
>>>> and expanded specfiles can't contain NUL bytes.  That can be fixed 
>>>> in format_commit_message() later and will then benefit users of 
>>>> git-log, too.
>>> Interesting. I however wonder if "specfile" is a good name for this 
>>> attribute, although I admit I do not think of anything better offhand.
>> "releasefile", perhaps?
> 
> Maybe we should not so much name it by purpose, but by function.  How 
> about "substformat" for the attribute name, and replacing any 
> $Format:blablub$ inside those files with something a la 
> --pretty=format:blablub?

I like the $Format:...$ notation.  How about naming the attribute
"template", as that's what a thus marked file is?

René

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[René Scharfe]

David Kastrup schrieb:
> I think a bit more layering would be helpful: when using git-svn, one
> would want to have things like $Id$ and $Date$ expanded, so maybe
> attribute specs like
> 
> somefile: expandmarkers="$Date: %aD$ $Id: ....$"
> 
> would be nice having.  In the case of git-svn, I would expect them to
> be generated from git-svn from the respective svn properties, so that
> the user is not bothered with figuring out the awful $Id$ and whatever
> strings.

Eek!  I'd rather shove this into a new layer below specfile/template
expansion, which would do something like that:

   s/\$Date:.*\$/$Format:$$Date: %aD$/

This way A) the complexity and ugliness affects only the users of these
fields, while $Format:...$ users are not adversely affected, and B) we
can first implement $Format:...$ handling and add the rest later.

In order to expand $Id$, git-archive would need to run some kind of
callback, right? :-/  Let's first see if such a thing is really needed.

René

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[René Scharfe]

Junio C Hamano schrieb:
>> Why did it take me that long to come up with such a simple patch?
>> There was a vacation and a feature freeze in between, but above all
>> I was only recently able to convince myself (using ugly code) that
>> format_commit_message() can indeed be made to expand placeholders
>> to git-describe strings..
> 
> Thanks.  Will take a look.

Here's that ugly code, by the way.  It adds two placeholders, %d for
description and %D for description depth.  Shortcomings of this code:
it adds three members to struct commit, it unconditionally computes
the description when format_commit_message() -- even if the format
string doesn't contain %d and %D, the patch is not nicely split up.
But it convinced me that describe *can* indeed be librarified. :-)

René


 Makefile           |    1 +
 builtin-describe.c |  219 +++------------------------------------------------
 cache.h            |    5 +
 commit.c           |   31 ++++++++
 commit.h           |    5 +
 describe.c         |  170 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 225 insertions(+), 206 deletions(-)

diff --git a/Makefile b/Makefile
index 51af531..7ec95f8 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,7 @@ DIFF_OBJS = \
 LIB_OBJS = \
 	blob.o commit.o connect.o csum-file.o cache-tree.o base85.o \
 	date.o diff-delta.o entry.o exec_cmd.o ident.o \
+	describe.o \
 	interpolate.o \
 	lockfile.o \
 	patch-ids.o \
diff --git a/builtin-describe.c b/builtin-describe.c
index 669110c..50ec08f 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -17,124 +17,13 @@ static int tags;	/* But allow any tags if --tags is specified */
 static int abbrev = DEFAULT_ABBREV;
 static int max_candidates = 10;
 
-struct commit_name {
-	int prio; /* annotated tag = 2, tag = 1, head = 0 */
-	char path[FLEX_ARRAY]; /* more */
-};
-static const char *prio_names[] = {
-	"head", "lightweight", "annotated",
-};
-
-static void add_to_known_names(const char *path,
-			       struct commit *commit,
-			       int prio)
-{
-	struct commit_name *e = commit->util;
-	if (!e || e->prio < prio) {
-		size_t len = strlen(path)+1;
-		free(e);
-		e = xmalloc(sizeof(struct commit_name) + len);
-		e->prio = prio;
-		memcpy(e->path, path, len);
-		commit->util = e;
-	}
-}
-
-static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)
-{
-	struct commit *commit = lookup_commit_reference_gently(sha1, 1);
-	struct object *object;
-	int prio;
-
-	if (!commit)
-		return 0;
-	object = parse_object(sha1);
-	/* If --all, then any refs are used.
-	 * If --tags, then any tags are used.
-	 * Otherwise only annotated tags are used.
-	 */
-	if (!prefixcmp(path, "refs/tags/")) {
-		if (object->type == OBJ_TAG)
-			prio = 2;
-		else
-			prio = 1;
-	}
-	else
-		prio = 0;
-
-	if (!all) {
-		if (!prio)
-			return 0;
-		if (!tags && prio < 2)
-			return 0;
-	}
-	add_to_known_names(all ? path + 5 : path + 10, commit, prio);
-	return 0;
-}
-
-struct possible_tag {
-	struct commit_name *name;
-	int depth;
-	int found_order;
-	unsigned flag_within;
-};
-
-static int compare_pt(const void *a_, const void *b_)
-{
-	struct possible_tag *a = (struct possible_tag *)a_;
-	struct possible_tag *b = (struct possible_tag *)b_;
-	if (a->name->prio != b->name->prio)
-		return b->name->prio - a->name->prio;
-	if (a->depth != b->depth)
-		return a->depth - b->depth;
-	if (a->found_order != b->found_order)
-		return a->found_order - b->found_order;
-	return 0;
-}
-
-static unsigned long finish_depth_computation(
-	struct commit_list **list,
-	struct possible_tag *best)
-{
-	unsigned long seen_commits = 0;
-	while (*list) {
-		struct commit *c = pop_commit(list);
-		struct commit_list *parents = c->parents;
-		seen_commits++;
-		if (c->object.flags & best->flag_within) {
-			struct commit_list *a = *list;
-			while (a) {
-				struct commit *i = a->item;
-				if (!(i->object.flags & best->flag_within))
-					break;
-				a = a->next;
-			}
-			if (!a)
-				break;
-		} else
-			best->depth++;
-		while (parents) {
-			struct commit *p = parents->item;
-			parse_commit(p);
-			if (!(p->object.flags & SEEN))
-				insert_by_date(p, list);
-			p->object.flags |= c->object.flags;
-			parents = parents->next;
-		}
-	}
-	return seen_commits;
-}
-
 static void describe(const char *arg, int last_one)
 {
 	unsigned char sha1[20];
-	struct commit *cmit, *gave_up_on = NULL;
-	struct commit_list *list;
+	struct commit *cmit;
 	static int initialized = 0;
-	struct commit_name *n;
-	struct possible_tag all_matches[MAX_TAGS];
-	unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
-	unsigned long seen_commits = 0;
+	char *name;
+	int depth = 0;
 
 	if (get_sha1(arg, sha1))
 		die("Not a valid object name %s", arg);
@@ -144,100 +33,23 @@ static void describe(const char *arg, int last_one)
 
 	if (!initialized) {
 		initialized = 1;
-		for_each_ref(get_name, NULL);
-	}
-
-	n = cmit->util;
-	if (n) {
-		printf("%s\n", n->path);
-		return;
+		load_commit_names(all ? 0 : (tags ? 1 : 2));
 	}
 
-	if (debug)
-		fprintf(stderr, "searching to describe %s\n", arg);
-
-	list = NULL;
-	cmit->object.flags = SEEN;
-	commit_list_insert(cmit, &list);
-	while (list) {
-		struct commit *c = pop_commit(&list);
-		struct commit_list *parents = c->parents;
-		seen_commits++;
-		n = c->util;
-		if (n) {
-			if (match_cnt < max_candidates) {
-				struct possible_tag *t = &all_matches[match_cnt++];
-				t->name = n;
-				t->depth = seen_commits - 1;
-				t->flag_within = 1u << match_cnt;
-				t->found_order = match_cnt;
-				c->object.flags |= t->flag_within;
-				if (n->prio == 2)
-					annotated_cnt++;
-			}
-			else {
-				gave_up_on = c;
-				break;
-			}
-		}
-		for (cur_match = 0; cur_match < match_cnt; cur_match++) {
-			struct possible_tag *t = &all_matches[cur_match];
-			if (!(c->object.flags & t->flag_within))
-				t->depth++;
-		}
-		if (annotated_cnt && !list) {
-			if (debug)
-				fprintf(stderr, "finished search at %s\n",
-					sha1_to_hex(c->object.sha1));
-			break;
-		}
-		while (parents) {
-			struct commit *p = parents->item;
-			parse_commit(p);
-			if (!(p->object.flags & SEEN))
-				insert_by_date(p, &list);
-			p->object.flags |= c->object.flags;
-			parents = parents->next;
-		}
-	}
-
-	if (!match_cnt)
+	name = describe_commit(cmit, max_candidates, &depth);
+	if (!name)
 		die("cannot describe '%s'", sha1_to_hex(cmit->object.sha1));
+	if (!all)
+		name += 5;
 
-	qsort(all_matches, match_cnt, sizeof(all_matches[0]), compare_pt);
-
-	if (gave_up_on) {
-		insert_by_date(gave_up_on, &list);
-		seen_commits--;
-	}
-	seen_commits += finish_depth_computation(&list, &all_matches[0]);
-	free_commit_list(list);
-
-	if (debug) {
-		for (cur_match = 0; cur_match < match_cnt; cur_match++) {
-			struct possible_tag *t = &all_matches[cur_match];
-			fprintf(stderr, " %-11s %8d %s\n",
-				prio_names[t->name->prio],
-				t->depth, t->name->path);
-		}
-		fprintf(stderr, "traversed %lu commits\n", seen_commits);
-		if (gave_up_on) {
-			fprintf(stderr,
-				"more than %i tags found; listed %i most recent\n"
-				"gave up search at %s\n",
-				max_candidates, max_candidates,
-				sha1_to_hex(gave_up_on->object.sha1));
-		}
-	}
-	if (abbrev == 0)
-		printf("%s\n", all_matches[0].name->path );
+	if (abbrev == 0 || depth == 0)
+		printf("%s\n", name);
 	else
-		printf("%s-%d-g%s\n", all_matches[0].name->path,
-		       all_matches[0].depth,
+		printf("%s-%d-g%s\n", name, depth,
 		       find_unique_abbrev(cmit->object.sha1, abbrev));
 
 	if (!last_one)
-		clear_commit_marks(cmit, -1);
+		clear_commit_name_flags(cmit);
 }
 
 int cmd_describe(int argc, const char **argv, const char *prefix)
@@ -263,13 +75,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			if (abbrev != 0 && (abbrev < MINIMUM_ABBREV || 40 < abbrev))
 				abbrev = DEFAULT_ABBREV;
 		}
-		else if (!prefixcmp(arg, "--candidates=")) {
+		else if (!prefixcmp(arg, "--candidates="))
 			max_candidates = strtoul(arg + 13, NULL, 10);
-			if (max_candidates < 1)
-				max_candidates = 1;
-			else if (max_candidates > MAX_TAGS)
-				max_candidates = MAX_TAGS;
-		}
 		else
 			usage(describe_usage);
 	}
diff --git a/cache.h b/cache.h
index 70abbd5..07ee149 100644
--- a/cache.h
+++ b/cache.h
@@ -600,4 +600,9 @@ extern int diff_auto_refresh_index;
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
 
+/* describe.c */
+struct commit;
+extern void load_commit_names(int min_prio);
+extern char *describe_commit(struct commit *cmit, int max_candidates, int *depthp);
+
 #endif /* CACHE_H */
diff --git a/commit.c b/commit.c
index dc5a064..d2343f2 100644
--- a/commit.c
+++ b/commit.c
@@ -455,6 +455,19 @@ void clear_commit_marks(struct commit *commit, unsigned int mark)
 	}
 }
 
+void clear_commit_name_flags(struct commit *commit)
+{
+	struct commit_list *parents;
+
+	commit->name_flags = 0;
+
+	for (parents = commit->parents; parents; parents = parents->next) {
+		/* Have we already cleared this? */
+		if (parents->item->name_flags)
+			clear_commit_name_flags(parents->item);
+	}
+}
+
 /*
  * Generic support for pretty-printing the header
  */
@@ -819,6 +832,8 @@ static long format_commit_message(const struct commit *commit,
 		{ "%Cblue" },	/* blue */
 		{ "%Creset" },	/* reset color */
 		{ "%n" },	/* newline */
+		{ "%d" },	/* description */
+		{ "%D" },	/* description depth */
 		{ "%m" },	/* left/right/bottom */
 	};
 	enum interp_index {
@@ -837,6 +852,7 @@ static long format_commit_message(const struct commit *commit,
 		IBODY,
 		IRED, IGREEN, IBLUE, IRESET_COLOR,
 		INEWLINE,
+		IDESC, IDESC_DEPTH,
 		ILEFT_RIGHT,
 	};
 	struct commit_list *p;
@@ -920,6 +936,21 @@ static long format_commit_message(const struct commit *commit,
 		if (!table[i].value)
 			interp_set_entry(table, i, "<unknown>");
 
+	{
+	struct commit *cmit = (struct commit *)commit;
+	char *name;
+	char tmp[20];
+	int depth;
+	load_commit_names(2);
+	name = describe_commit(cmit, 10, &depth);
+	if (!name)
+		name = "";
+	sprintf(tmp, "%d", depth);
+	interp_set_entry(table, IDESC, name);
+	interp_set_entry(table, IDESC_DEPTH, tmp);
+	clear_commit_name_flags(cmit);
+	}
+
 	do {
 		char *buf = *buf_p;
 		unsigned long space = *space_p;
diff --git a/commit.h b/commit.h
index 467872e..69b6d67 100644
--- a/commit.h
+++ b/commit.h
@@ -17,6 +17,9 @@ struct commit {
 	struct commit_list *parents;
 	struct tree *tree;
 	char *buffer;
+	char *name;
+	unsigned int name_flags;
+	char name_prio;
 };
 
 extern int save_commit_buffer;
@@ -72,6 +75,8 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
 struct commit *pop_commit(struct commit_list **stack);
 
 void clear_commit_marks(struct commit *commit, unsigned int mark);
+void clear_commit_name_flags(struct commit *commit);
+
 
 /*
  * Performs an in-place topological sort of list supplied.
diff --git a/describe.c b/describe.c
new file mode 100644
index 0000000..acd4517
--- /dev/null
+++ b/describe.c
@@ -0,0 +1,170 @@
+#include "cache.h"
+#include "commit.h"
+#include "tag.h"
+#include "refs.h"
+
+#define SEEN		(1u<<0)
+#define MAX_TAGS	(FLAG_BITS - 1)
+
+struct possible_tag {
+	char *name;
+	int name_prio;
+	int depth;
+	int found_order;
+	unsigned flag_within;
+};
+
+static int get_name(const char *path, const unsigned char *sha1, int flag,
+                    void *cb_data)
+{
+	struct commit *commit = lookup_commit_reference_gently(sha1, 1);
+	if (commit) {
+		struct object *object = parse_object(sha1);
+		int min_prio = *((int *)cb_data);
+		int prio = 0;
+
+		if (!prefixcmp(path, "refs/tags/")) {
+			if (object->type == OBJ_TAG)
+				prio = 2;
+			else
+				prio = 1;
+		}
+
+		if (prio >= min_prio &&
+		    (!commit->name || commit->name_prio < prio)) {
+			free(commit->name);
+			commit->name = xstrdup(path + 5);
+			commit->name_prio = prio;
+		}
+	}
+	return 0;
+}
+
+void load_commit_names(int min_prio)
+{
+	for_each_ref(get_name, &min_prio);
+}
+
+static int compare_pt(const void *a_, const void *b_)
+{
+	struct possible_tag *a = (struct possible_tag *)a_;
+	struct possible_tag *b = (struct possible_tag *)b_;
+	if (a->name_prio != b->name_prio)
+		return b->name_prio - a->name_prio;
+	if (a->depth != b->depth)
+		return a->depth - b->depth;
+	if (a->found_order != b->found_order)
+		return a->found_order - b->found_order;
+	return 0;
+}
+
+static unsigned long finish_depth_computation(
+	struct commit_list **list,
+	struct possible_tag *best)
+{
+	unsigned long seen_commits = 0;
+	while (*list) {
+		struct commit *c = pop_commit(list);
+		struct commit_list *parents = c->parents;
+		seen_commits++;
+		if (c->name_flags & best->flag_within) {
+			struct commit_list *a = *list;
+			while (a) {
+				struct commit *i = a->item;
+				if (!(i->name_flags & best->flag_within))
+					break;
+				a = a->next;
+			}
+			if (!a)
+				break;
+		} else
+			best->depth++;
+		while (parents) {
+			struct commit *p = parents->item;
+			parse_commit(p);
+			if (!(p->name_flags & SEEN))
+				insert_by_date(p, list);
+			p->name_flags |= c->name_flags;
+			parents = parents->next;
+		}
+	}
+	return seen_commits;
+}
+
+char *describe_commit(struct commit *cmit, int max_candidates, int *depthp)
+{
+	struct commit *gave_up_on = NULL;
+	struct commit_list *list;
+	struct possible_tag all_matches[MAX_TAGS];
+	unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
+	unsigned long seen_commits = 0;
+
+	if (cmit->name) {
+		*depthp = 0;
+		return cmit->name;
+	}
+
+	if (max_candidates < 1)
+		max_candidates = 1;
+	else if (max_candidates > MAX_TAGS)
+		max_candidates = MAX_TAGS;
+
+	list = NULL;
+	cmit->name_flags = SEEN;
+	commit_list_insert(cmit, &list);
+	while (list) {
+		struct commit *c = pop_commit(&list);
+		struct commit_list *parents = c->parents;
+		seen_commits++;
+		if (c->name) {
+			if (match_cnt < max_candidates) {
+				struct possible_tag *t = &all_matches[match_cnt++];
+				t->name = c->name;
+				t->name_prio = c->name_prio;
+				t->depth = seen_commits - 1;
+				t->flag_within = 1u << match_cnt;
+				t->found_order = match_cnt;
+				c->name_flags |= t->flag_within;
+				if (c->name_prio == 2)
+					annotated_cnt++;
+			}
+			else {
+				gave_up_on = c;
+				break;
+			}
+		}
+		for (cur_match = 0; cur_match < match_cnt; cur_match++) {
+			struct possible_tag *t = &all_matches[cur_match];
+			if (!(c->name_flags & t->flag_within))
+				t->depth++;
+		}
+		if (annotated_cnt && !list)
+			break;
+		while (parents) {
+			struct commit *p = parents->item;
+			parse_commit(p);
+			if (!(p->name_flags & SEEN))
+				insert_by_date(p, &list);
+			p->name_flags |= c->name_flags;
+			parents = parents->next;
+		}
+	}
+
+	if (!match_cnt) {
+		free_commit_list(list);
+		*depthp = -1;
+		return NULL;
+	}
+
+	qsort(all_matches, match_cnt, sizeof(all_matches[0]), compare_pt);
+
+	if (gave_up_on) {
+		insert_by_date(gave_up_on, &list);
+		seen_commits--;
+	}
+	seen_commits += finish_depth_computation(&list, &all_matches[0]);
+	free_commit_list(list);
+
+	*depthp = all_matches[0].depth;
+	return all_matches[0].name;
+}

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Johannes Schindelin]

Hi,

On Wed, 5 Sep 2007, Ren? Scharfe wrote:

> Johannes Schindelin schrieb:
> 
> > On Tue, 4 Sep 2007, Andreas Ericsson wrote:
> > 
> >> Junio C Hamano wrote:
> >>> Ren? Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> >>>
> >>>> The attribute is useful for creating auto-updating specfiles.  It is 
> >>>> limited by the underlying function format_commit_message(), though. 
> >>>> E.g. currently there is no placeholder for git-describe like output, 
> >>>> and expanded specfiles can't contain NUL bytes.  That can be fixed 
> >>>> in format_commit_message() later and will then benefit users of 
> >>>> git-log, too.
> >>> Interesting. I however wonder if "specfile" is a good name for this 
> >>> attribute, although I admit I do not think of anything better offhand.
> >> "releasefile", perhaps?
> > 
> > Maybe we should not so much name it by purpose, but by function.  How 
> > about "substformat" for the attribute name, and replacing any 
> > $Format:blablub$ inside those files with something a la 
> > --pretty=format:blablub?
> 
> I like the $Format:...$ notation.  How about naming the attribute
> "template", as that's what a thus marked file is?

Unless somebody comes up with an even better name, yes (I like "template" 
better than "substformat" or "releasefile", but I still think that 
"template" is not descriptive enough).

Ciao,
Dscho

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Here's that ugly code, by the way.  It adds two placeholders, %d for
> description and %D for description depth.  Shortcomings of this code:
> it adds three members to struct commit, it unconditionally computes
> the description when format_commit_message() -- even if the format
> string doesn't contain %d and %D, the patch is not nicely split up.
> But it convinced me that describe *can* indeed be librarified. :-)

Yeah, unconditional computation for %d/%D is really a killer.
Otherwise the change itself does not necessarily look too ugly,
though.

Re: [PATCH 2/3] archive: specfile support (--pretty=format: in archive files)

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

>> Maybe we should not so much name it by purpose, but by function.  How 
>> about "substformat" for the attribute name, and replacing any 
>> $Format:blablub$ inside those files with something a la 
>> --pretty=format:blablub?
>
> I like the $Format:...$ notation.  How about naming the attribute
> "template", as that's what a thus marked file is?

Sounds good, although I suspect "template" might confuse newbies
that checkout may apply the substitution as well.  How about
something with "export" in it?  export-subst, perhaps?

[PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[René Scharfe]

As suggested by Johannes, --pretty=format: placeholders in specfiles
need to be wrapped in $Format:...$ now.  This syntax change restricts
the expansion of placeholders and makes it easier to use with files
that contain non-placeholder percent signs.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/gitattributes.txt |    5 +++-
 builtin-archive.c               |   52 +++++++++++++++++++++++++++++++++++---
 t/t5000-tar-tree.sh             |    4 +-
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 47a621b..37b3be8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -432,7 +432,10 @@ several placeholders when adding this file to an archive.  The
 expansion depends on the availability of a commit ID, i.e. if
 gitlink:git-archive[1] has been given a tree instead of a commit or a
 tag then no replacement will be done.  The placeholders are the same
-as those for the option `--pretty=format:` of gitlink:git-log[1].
+as those for the option `--pretty=format:` of gitlink: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.
 
 
 GIT
diff --git a/builtin-archive.c b/builtin-archive.c
index faccce3..a8a0f01 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -81,14 +81,58 @@ static int run_remote_archiver(const char *remote, int argc,
 	return !!rv;
 }
 
+static void *format_specfile(const struct commit *commit, const char *format,
+                             unsigned long *sizep)
+{
+	unsigned long len = *sizep, result_len = 0;
+	const char *a = format;
+	char *result = NULL;
+
+	for (;;) {
+		const char *b, *c;
+		char *fmt, *formatted = NULL;
+		unsigned long a_len, fmt_len, formatted_len, allocated = 0;
+
+		b = memchr(a, '$', len);
+		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
+			break;
+		c = memchr(b + 8, '$', len - 8);
+		if (!c)
+			break;
+
+		a_len = b - a;
+		fmt_len = c - b - 8;
+		fmt = xmalloc(fmt_len + 1);
+		memcpy(fmt, b + 8, fmt_len);
+		fmt[fmt_len] = '\0';
+
+		formatted_len = format_commit_message(commit, fmt, &formatted,
+		                                      &allocated);
+		result = xrealloc(result, result_len + a_len + formatted_len);
+		memcpy(result + result_len, a, a_len);
+		memcpy(result + result_len + a_len, formatted, formatted_len);
+		result_len += a_len + formatted_len;
+		len -= c + 1 - a;
+		a = c + 1;
+	}
+
+	if (result && len) {
+		result = xrealloc(result, result_len + len);
+		memcpy(result + result_len, a, len);
+		result_len += len;
+	}
+
+	*sizep = result_len;
+
+	return result;
+}
+
 static void *convert_to_archive(const char *path,
                                 const void *src, unsigned long *sizep,
                                 const struct commit *commit)
 {
 	static struct git_attr *attr_specfile;
 	struct git_attr_check check[1];
-	char *interpolated = NULL;
-	unsigned long allocated = 0;
 
 	if (!commit)
 		return NULL;
@@ -102,9 +146,7 @@ static void *convert_to_archive(const char *path,
 	if (!ATTR_TRUE(check[0].value))
 		return NULL;
 
-	*sizep = format_commit_message(commit, src, &interpolated, &allocated);
-
-	return interpolated;
+	return format_specfile(commit, src, sizep);
 }
 
 void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 3d5d01b..6e89e07 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -36,7 +36,7 @@ test_expect_success \
      echo simple textfile >a/a &&
      mkdir a/bin &&
      cp /bin/sh a/bin &&
-     printf "%s" "$SPECFILEFORMAT" >a/specfile &&
+     printf "A\$Format:%s\$O" "$SPECFILEFORMAT" >a/specfile &&
      ln -s a a/l1 &&
      (p=long_path_to_a_file && cd a &&
       for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -119,7 +119,7 @@ test_expect_success \
 
 test_expect_success \
      'validate specfile contents' \
-     'git log --max-count=1 "--pretty=format:$SPECFILEFORMAT" HEAD \
+     'git log --max-count=1 "--pretty=format:A${SPECFILEFORMAT}O" HEAD \
       >f/a/specfile.expected &&
       diff f/a/specfile.expected f/a/specfile'
 
-- 
1.5.3

[PATCH 5/3] archive: rename attribute specfile to export-subst

[René Scharfe]

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>>> Maybe we should not so much name it by purpose, but by function.  How 
>>> about "substformat" for the attribute name, and replacing any 
>>> $Format:blablub$ inside those files with something a la 
>>> --pretty=format:blablub?
>> I like the $Format:...$ notation.  How about naming the attribute
>> "template", as that's what a thus marked file is?
> 
> Sounds good, although I suspect "template" might confuse newbies
> that checkout may apply the substitution as well.  How about
> something with "export" in it?  export-subst, perhaps?

Well, including "export" in the name makes sense, yes.  I can't come up
with a better name, let's take this.

--- snip! ---
As suggested by Junio and Johannes, change the name of the former
attribute specfile to export-subst to indicate its function rather
than purpose and to make clear that it is not applied to working tree
files.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 Documentation/gitattributes.txt |    6 +++---
 builtin-archive.c               |   14 +++++++-------
 t/t5000-tar-tree.sh             |   18 +++++++++---------
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 37b3be8..d0e951e 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -424,10 +424,10 @@ frotz	unspecified
 Creating an archive
 ~~~~~~~~~~~~~~~~~~~
 
-`specfile`
-^^^^^^^^^^
+`export-subst`
+^^^^^^^^^^^^^^
 
-If the attribute `specfile` is set for a file then git will expand
+If the attribute `export-subst` is set for a file then git will expand
 several placeholders when adding this file to an archive.  The
 expansion depends on the availability of a commit ID, i.e. if
 gitlink:git-archive[1] has been given a tree instead of a commit or a
diff --git a/builtin-archive.c b/builtin-archive.c
index a8a0f01..af14837 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -81,8 +81,8 @@ static int run_remote_archiver(const char *remote, int argc,
 	return !!rv;
 }
 
-static void *format_specfile(const struct commit *commit, const char *format,
-                             unsigned long *sizep)
+static void *format_subst(const struct commit *commit, const char *format,
+                          unsigned long *sizep)
 {
 	unsigned long len = *sizep, result_len = 0;
 	const char *a = format;
@@ -131,22 +131,22 @@ static void *convert_to_archive(const char *path,
                                 const void *src, unsigned long *sizep,
                                 const struct commit *commit)
 {
-	static struct git_attr *attr_specfile;
+	static struct git_attr *attr_export_subst;
 	struct git_attr_check check[1];
 
 	if (!commit)
 		return NULL;
 
-        if (!attr_specfile)
-                attr_specfile = git_attr("specfile", 8);
+        if (!attr_export_subst)
+                attr_export_subst = git_attr("export-subst", 12);
 
-	check[0].attr = attr_specfile;
+	check[0].attr = attr_export_subst;
 	if (git_checkattr(path, ARRAY_SIZE(check), check))
 		return NULL;
 	if (!ATTR_TRUE(check[0].value))
 		return NULL;
 
-	return format_specfile(commit, src, sizep);
+	return format_subst(commit, src, sizep);
 }
 
 void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 6e89e07..42e28ab 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -28,7 +28,7 @@ commit id embedding:
 TAR=${TAR:-tar}
 UNZIP=${UNZIP:-unzip}
 
-SPECFILEFORMAT=%H%n
+SUBSTFORMAT=%H%n
 
 test_expect_success \
     'populate workdir' \
@@ -36,7 +36,7 @@ test_expect_success \
      echo simple textfile >a/a &&
      mkdir a/bin &&
      cp /bin/sh a/bin &&
-     printf "A\$Format:%s\$O" "$SPECFILEFORMAT" >a/specfile &&
+     printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile &&
      ln -s a a/l1 &&
      (p=long_path_to_a_file && cd a &&
       for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -108,20 +108,20 @@ test_expect_success \
     'diff -r a c/prefix/a'
 
 test_expect_success \
-    'create an archive with a specfile' \
-    'echo specfile specfile >a/.gitattributes &&
+    'create an archive with a substfile' \
+    'echo substfile export-subst >a/.gitattributes &&
      git archive HEAD >f.tar &&
      rm a/.gitattributes'
 
 test_expect_success \
-    'extract specfile' \
+    'extract substfile' \
     '(mkdir f && cd f && $TAR xf -) <f.tar'
 
 test_expect_success \
-     'validate specfile contents' \
-     'git log --max-count=1 "--pretty=format:A${SPECFILEFORMAT}O" HEAD \
-      >f/a/specfile.expected &&
-      diff f/a/specfile.expected f/a/specfile'
+     'validate substfile contents' \
+     'git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
+      >f/a/substfile.expected &&
+      diff f/a/substfile.expected f/a/substfile'
 
 test_expect_success \
     'git archive --format=zip' \

Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1351 bytes --]

Hi,

On Thu, 6 Sep 2007, René Scharfe wrote:

> As suggested by Johannes, --pretty=format: placeholders in specfiles 
> need to be wrapped in $Format:...$ now.

Thanks.

> diff --git a/builtin-archive.c b/builtin-archive.c
> index faccce3..a8a0f01 100644
> --- a/builtin-archive.c
> +++ b/builtin-archive.c
> @@ -81,14 +81,58 @@ static int run_remote_archiver(const char *remote, int argc,
>  	return !!rv;
>  }
>  
> +static void *format_specfile(const struct commit *commit, const char *format,
> +                             unsigned long *sizep)

Should this not be "char *buffer" instead of "const char *format"?  Or 
even better: a "struct strbuf *"?

> +{
> +	unsigned long len = *sizep, result_len = 0;
> +	const char *a = format;
> +	char *result = NULL;
> +
> +	for (;;) {
> +		const char *b, *c;
> +		char *fmt, *formatted = NULL;
> +		unsigned long a_len, fmt_len, formatted_len, allocated = 0;

Maybe initialise formatted_len, just to be on the safe side?

> +
> +		b = memchr(a, '$', len);
> +		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
> +			break;

Wouldn't memmem(buffer, len, "$Format:", 8) be better here?

A general comment: since you plan to output the result into a file anyway, 
it should be even easier to avoid realloc(), and do a 
print_formatted_specfile() instead of a format_specfile(), no?

Ciao,
Dscho

Re: [PATCH 5/3] archive: rename attribute specfile to export-subst

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 572 bytes --]

Hi,

On Thu, 6 Sep 2007, René Scharfe wrote:

> As suggested by Junio and Johannes, change the name of the former
> attribute specfile to export-subst to indicate its function rather
> than purpose and to make clear that it is not applied to working tree
> files.
> 
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

ACK!

(Even if I did not really suggest "export-subst", which I like very 
much...)

The bigger question is now if these two patches should be folded back into 
your original patch series, or stand alone as commits of their own...

Ciao,
Dscho

Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[René Scharfe]

Johannes Schindelin schrieb:
>> +static void *format_specfile(const struct commit *commit, const char *format,
>> +                             unsigned long *sizep)
> 
> Should this not be "char *buffer" instead of "const char *format"?  Or 
> even better: a "struct strbuf *"?

const is correct, because the data in the buffer shouldn't be (and
isn't) modified.

"buffer" is a generic term; "format" on the other hand indicates the
meaning of the data within the buffer.

Input and output might indeed be passed along as struct strbuf, even
more so since the new API encourages its use as generic buffer (format
can contain NULs).

>> +{
>> +	unsigned long len = *sizep, result_len = 0;
>> +	const char *a = format;
>> +	char *result = NULL;
>> +
>> +	for (;;) {
>> +		const char *b, *c;
>> +		char *fmt, *formatted = NULL;
>> +		unsigned long a_len, fmt_len, formatted_len, allocated = 0;
> 
> Maybe initialise formatted_len, just to be on the safe side?

I wish I could use C99 syntax and move its declaration down to its first
use, then it would be obvious that formatted_len is never used without
initialization.

>> +
>> +		b = memchr(a, '$', len);
>> +		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>> +			break;
> 
> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?

Oh, that's a nice GNU extension, didn't know it before.  We might import
it to compat etc., but I think that's better left for a follow-up patch.

> A general comment: since you plan to output the result into a file anyway, 
> it should be even easier to avoid realloc(), and do a 
> print_formatted_specfile() instead of a format_specfile(), no?

Hmm, not sure what you mean.  At least archive-tar needs the expanded
contents in a buffer (not immediately written to stdout) because it
tries to mimic a real tar and always writes in blocks of 10k and
therefore needs to buffer the output.

Thanks!
René

Re: [PATCH 5/3] archive: rename attribute specfile to export-subst

[René Scharfe]

Johannes Schindelin schrieb:
> The bigger question is now if these two patches should be folded back
> into your original patch series, or stand alone as commits of their
> own...

Yes, indeed.  I submitted them as add-on patches because I noticed the
first three are already in next, but I sure can make a fresh three-part
series based on master.  Junio?

Thanks,
René

Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[René Scharfe]

René Scharfe schrieb:
> Johannes Schindelin schrieb:
>>> +
>>> +		b = memchr(a, '$', len);
>>> +		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
>>> +			break;
>> Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
> 
> Oh, that's a nice GNU extension, didn't know it before.  We might import
> it to compat etc., but I think that's better left for a follow-up patch.

Just noticed: if the memcmp() above finds a difference, the code should
*not* break out of the loop.  Ahem.  Perhaps I should first add memmem()
after all...

René

Re: [PATCH 5/3] archive: rename attribute specfile to export-subst

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The bigger question is now if these two patches should be folded back into 
> your original patch series, or stand alone as commits of their own...

That is no brainer, as there is a simple and hard rule that any
topic already in 'next' are not to be rewound ever.  Follow-up
patches are the right thing to do in this case.

[PATCH 3.5/3] add memmem()

[René Scharfe]

memmem() is a nice GNU extension for searching a length limited string
in another one.

This compat version is based on the version found in glibc 2.2 (GPL 2);
I only removed the optimization of checking the first char by hand, and
generally tried to keep the code simple.  We can add it back if memcmp
shows up high in a profile, but for now I prefer to keep it (almost
trivially) simple.

Since I don't really know which platforms beside those with a glibc
have their own memmem(), I used a heuristic: if NO_STRCASESTR is set,
then NO_MEMMEM is set, too.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Makefile          |   11 +++++++++++
 compat/memmem.c   |   29 +++++++++++++++++++++++++++++
 git-compat-util.h |    6 ++++++
 3 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100644 compat/memmem.c

diff --git a/Makefile b/Makefile
index 51af531..bae073f 100644
--- a/Makefile
+++ b/Makefile
@@ -28,6 +28,8 @@ all::
 #
 # Define NO_STRCASESTR if you don't have strcasestr.
 #
+# Define NO_MEMMEM if you don't have memmem.
+#
 # Define NO_STRLCPY if you don't have strlcpy.
 #
 # Define NO_STRTOUMAX if you don't have strtoumax in the C library.
@@ -402,6 +404,7 @@ ifeq ($(uname_S),SunOS)
 	NEEDS_NSL = YesPlease
 	SHELL_PATH = /bin/bash
 	NO_STRCASESTR = YesPlease
+	NO_MEMMEM = YesPlease
 	NO_HSTRERROR = YesPlease
 	ifeq ($(uname_R),5.8)
 		NEEDS_LIBICONV = YesPlease
@@ -424,6 +427,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_D_TYPE_IN_DIRENT = YesPlease
 	NO_D_INO_IN_DIRENT = YesPlease
 	NO_STRCASESTR = YesPlease
+	NO_MEMMEM = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -442,6 +446,7 @@ ifeq ($(uname_S),FreeBSD)
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
+	NO_MEMMEM = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
@@ -456,6 +461,7 @@ ifeq ($(uname_S),NetBSD)
 endif
 ifeq ($(uname_S),AIX)
 	NO_STRCASESTR=YesPlease
+	NO_MEMMEM = YesPlease
 	NO_STRLCPY = YesPlease
 	NEEDS_LIBICONV=YesPlease
 endif
@@ -467,6 +473,7 @@ ifeq ($(uname_S),IRIX64)
 	NO_IPV6=YesPlease
 	NO_SETENV=YesPlease
 	NO_STRCASESTR=YesPlease
+	NO_MEMMEM = YesPlease
 	NO_STRLCPY = YesPlease
 	NO_SOCKADDR_STORAGE=YesPlease
 	SHELL_PATH=/usr/gnu/bin/bash
@@ -661,6 +668,10 @@ ifdef NO_HSTRERROR
 	COMPAT_CFLAGS += -DNO_HSTRERROR
 	COMPAT_OBJS += compat/hstrerror.o
 endif
+ifdef NO_MEMMEM
+	COMPAT_CFLAGS += -DNO_MEMMEM
+	COMPAT_OBJS += compat/memmem.o
+endif
 
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks
diff --git a/compat/memmem.c b/compat/memmem.c
new file mode 100644
index 0000000..cd0d877
--- /dev/null
+++ b/compat/memmem.c
@@ -0,0 +1,29 @@
+#include "../git-compat-util.h"
+
+void *gitmemmem(const void *haystack, size_t haystack_len,
+                const void *needle, size_t needle_len)
+{
+	const char *begin = haystack;
+	const char *last_possible = begin + haystack_len - needle_len;
+
+	/*
+	 * The first occurrence of the empty string is deemed to occur at
+	 * the beginning of the string.
+	 */
+	if (needle_len == 0)
+		return (void *)begin;
+
+	/*
+	 * Sanity check, otherwise the loop might search through the whole
+	 * memory.
+	 */
+	if (haystack_len < needle_len)
+		return NULL;
+
+	for (; begin <= last_possible; begin++) {
+		if (!memcmp(begin, needle, needle_len))
+			return (void *)begin;
+	}
+
+	return NULL;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index ca0a597..1bfbdeb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -172,6 +172,12 @@ extern uintmax_t gitstrtoumax(const char *, char **, int);
 extern const char *githstrerror(int herror);
 #endif
 
+#ifdef NO_MEMMEM
+#define memmem gitmemmem
+void *gitmemmem(const void *haystack, size_t haystacklen,
+                const void *needle, size_t needlelen);
+#endif
+
 extern void release_pack_memory(size_t, int);
 
 static inline char* xstrdup(const char *str)
-- 
1.5.3

[PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR" (take 2)

[René Scharfe]

As suggested by Johannes, --pretty=format: placeholders in specfiles
need to be wrapped in $Format:...$ now.  This syntax change restricts
the expansion of placeholders and makes it easier to use with files
that contain non-placeholder percent signs.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
This replacement patch fixes a bug in the "$Format:" search logic.  It
now uses memmem() and thus depends on patch 3.5 which introduces this
function.  Patch 5 still applies unchanged.

 Documentation/gitattributes.txt |    5 +++-
 builtin-archive.c               |   52 +++++++++++++++++++++++++++++++++++---
 t/t5000-tar-tree.sh             |    4 +-
 3 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 47a621b..37b3be8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -432,7 +432,10 @@ several placeholders when adding this file to an archive.  The
 expansion depends on the availability of a commit ID, i.e. if
 gitlink:git-archive[1] has been given a tree instead of a commit or a
 tag then no replacement will be done.  The placeholders are the same
-as those for the option `--pretty=format:` of gitlink:git-log[1].
+as those for the option `--pretty=format:` of gitlink: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.
 
 
 GIT
diff --git a/builtin-archive.c b/builtin-archive.c
index faccce3..65bf9cb 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -81,14 +81,58 @@ static int run_remote_archiver(const char *remote, int argc,
 	return !!rv;
 }
 
+static void *format_specfile(const struct commit *commit, const char *format,
+                             unsigned long *sizep)
+{
+	unsigned long len = *sizep, result_len = 0;
+	const char *a = format;
+	char *result = NULL;
+
+	for (;;) {
+		const char *b, *c;
+		char *fmt, *formatted = NULL;
+		unsigned long a_len, fmt_len, formatted_len, allocated = 0;
+
+		b = memmem(a, len, "$Format:", 8);
+		if (!b || a + len < b + 9)
+			break;
+		c = memchr(b + 8, '$', len - 8);
+		if (!c)
+			break;
+
+		a_len = b - a;
+		fmt_len = c - b - 8;
+		fmt = xmalloc(fmt_len + 1);
+		memcpy(fmt, b + 8, fmt_len);
+		fmt[fmt_len] = '\0';
+
+		formatted_len = format_commit_message(commit, fmt, &formatted,
+		                                      &allocated);
+		result = xrealloc(result, result_len + a_len + formatted_len);
+		memcpy(result + result_len, a, a_len);
+		memcpy(result + result_len + a_len, formatted, formatted_len);
+		result_len += a_len + formatted_len;
+		len -= c + 1 - a;
+		a = c + 1;
+	}
+
+	if (result && len) {
+		result = xrealloc(result, result_len + len);
+		memcpy(result + result_len, a, len);
+		result_len += len;
+	}
+
+	*sizep = result_len;
+
+	return result;
+}
+
 static void *convert_to_archive(const char *path,
                                 const void *src, unsigned long *sizep,
                                 const struct commit *commit)
 {
 	static struct git_attr *attr_specfile;
 	struct git_attr_check check[1];
-	char *interpolated = NULL;
-	unsigned long allocated = 0;
 
 	if (!commit)
 		return NULL;
@@ -102,9 +146,7 @@ static void *convert_to_archive(const char *path,
 	if (!ATTR_TRUE(check[0].value))
 		return NULL;
 
-	*sizep = format_commit_message(commit, src, &interpolated, &allocated);
-
-	return interpolated;
+	return format_specfile(commit, src, sizep);
 }
 
 void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 3d5d01b..6e89e07 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -36,7 +36,7 @@ test_expect_success \
      echo simple textfile >a/a &&
      mkdir a/bin &&
      cp /bin/sh a/bin &&
-     printf "%s" "$SPECFILEFORMAT" >a/specfile &&
+     printf "A\$Format:%s\$O" "$SPECFILEFORMAT" >a/specfile &&
      ln -s a a/l1 &&
      (p=long_path_to_a_file && cd a &&
       for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
@@ -119,7 +119,7 @@ test_expect_success \
 
 test_expect_success \
      'validate specfile contents' \
-     'git log --max-count=1 "--pretty=format:$SPECFILEFORMAT" HEAD \
+     'git log --max-count=1 "--pretty=format:A${SPECFILEFORMAT}O" HEAD \
       >f/a/specfile.expected &&
       diff f/a/specfile.expected f/a/specfile'
 
-- 
1.5.3

Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Just noticed: if the memcmp() above finds a difference, the code should
> *not* break out of the loop.  Ahem.  Perhaps I should first add memmem()
> after all...

That sounds like a sensible thing to do.  

Will drop this copy and use updated 4/3 and 3.5/3; thanks.

Re: [PATCH 4/3] archive: specfile syntax change: "$Format:%PLCHLDR$" instead of just "%PLCHLDR"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 925 bytes --]

Hi,

On Thu, 6 Sep 2007, René Scharfe wrote:

> Johannes Schindelin schrieb:
> >> +
> >> +		b = memchr(a, '$', len);
> >> +		if (!b || a + len < b + 9 || memcmp(b + 1, "Format:", 7))
> >> +			break;
> > 
> > Wouldn't memmem(buffer, len, "$Format:", 8) be better here?
> 
> Oh, that's a nice GNU extension, didn't know it before.

Oh sorry, I didn't even realise that this is a GNU extension...

> > A general comment: since you plan to output the result into a file 
> > anyway, it should be even easier to avoid realloc(), and do a 
> > print_formatted_specfile() instead of a format_specfile(), no?
> 
> Hmm, not sure what you mean.  At least archive-tar needs the expanded 
> contents in a buffer (not immediately written to stdout) because it 
> tries to mimic a real tar and always writes in blocks of 10k and 
> therefore needs to buffer the output.

Yeah, I missed that.  Thanks for explaining it to me!

Ciao,
Dscho

Re: [PATCH 5/3] archive: rename attribute specfile to export-subst

[Johannes Schindelin]

Hi,

On Thu, 6 Sep 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The bigger question is now if these two patches should be folded back 
> > into your original patch series, or stand alone as commits of their 
> > own...
> 
> That is no brainer, as there is a simple and hard rule that any topic 
> already in 'next' are not to be rewound ever.  Follow-up patches are the 
> right thing to do in this case.

Right.  At the time I suggested it, I was not aware that the patches were 
in next already.

Sorry,
Dscho