[PATCH 6/6] archive: remove extra arguments parsing code

[PATCH 6/6] archive: remove extra arguments parsing code

[René Scharfe]

Replace the code that calls backend specific argument parsers by a
simple flag mechanism.  This reduces code size and complexity.

We can add back such a mechanism (based on incremental parse_opt(),
perhaps) when we need it.  The compression level parameter, though,
is going to be shared by future compressing backends like tgz.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-zip.c     |   13 -------------
 archive.h         |    6 +-----
 builtin-archive.c |   29 ++++++++++++++++-------------
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 8131289..d56e5cf 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -282,16 +282,3 @@ int write_zip_archive(struct archiver_args *args)
 
 	return err;
 }
-
-void *parse_extra_zip_args(int argc, const char **argv)
-{
-	for (; argc > 0; argc--, argv++) {
-		const char *arg = argv[0];
-
-		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0')
-			zlib_compression_level = arg[1] - '0';
-		else
-			die("Unknown argument for zip format: %s", arg);
-	}
-	return NULL;
-}
diff --git a/archive.h b/archive.h
index 88ee3be..96bb1cd 100644
--- a/archive.h
+++ b/archive.h
@@ -13,19 +13,16 @@ struct archiver_args {
 	time_t time;
 	const char **pathspec;
 	unsigned int verbose : 1;
-	void *extra;
 };
 
 typedef int (*write_archive_fn_t)(struct archiver_args *);
 
-typedef void *(*parse_extra_args_fn_t)(int argc, const char **argv);
-
 typedef int (*write_archive_entry_fn_t)(struct archiver_args *args, const unsigned char *sha1, const char *path, size_t pathlen, unsigned int mode, void *buffer, unsigned long size);
 
 struct archiver {
 	const char *name;
 	write_archive_fn_t write_archive;
-	parse_extra_args_fn_t parse_extra;
+	unsigned int flags;
 };
 
 extern int parse_archive_args(int argc, const char **argv, const struct archiver **ar, struct archiver_args *args);
@@ -41,7 +38,6 @@ extern void parse_pathspec_arg(const char **pathspec,
  */
 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 int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
 
diff --git a/builtin-archive.c b/builtin-archive.c
index e7f4ec6..88204bf 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -15,9 +15,11 @@
 static const char archive_usage[] = \
 "git-archive --format=<fmt> [--prefix=<prefix>/] [--verbose] [<extra>] <tree-ish> [path...]";
 
+#define USES_ZLIB_COMPRESSION 1
+
 const struct archiver archivers[] = {
-	{ "tar", write_tar_archive, NULL },
-	{ "zip", write_zip_archive, parse_extra_zip_args },
+	{ "tar", write_tar_archive },
+	{ "zip", write_zip_archive, USES_ZLIB_COMPRESSION },
 };
 
 static int run_remote_archiver(const char *remote, int argc,
@@ -137,10 +139,9 @@ void parse_treeish_arg(const char **argv, struct archiver_args *ar_args,
 int parse_archive_args(int argc, const char **argv, const struct archiver **ar,
 		struct archiver_args *args)
 {
-	const char *extra_argv[MAX_EXTRA_ARGS];
-	int extra_argc = 0;
 	const char *format = "tar";
 	const char *base = "";
+	int compression_level = -1;
 	int verbose = 0;
 	int i;
 
@@ -168,12 +169,12 @@ int parse_archive_args(int argc, const char **argv, const struct archiver **ar,
 			i++;
 			break;
 		}
-		if (arg[0] == '-') {
-			if (extra_argc > MAX_EXTRA_ARGS - 1)
-				die("Too many extra options");
-			extra_argv[extra_argc++] = arg;
+		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
+			compression_level = arg[1] - '0';
 			continue;
 		}
+		if (arg[0] == '-')
+			die("Unknown argument: %s", arg);
 		break;
 	}
 
@@ -184,11 +185,13 @@ int parse_archive_args(int argc, const char **argv, const struct archiver **ar,
 	if (!*ar)
 		die("Unknown archive format '%s'", format);
 
-	if (extra_argc) {
-		if (!(*ar)->parse_extra)
-			die("'%s' format does not handle %s",
-			    (*ar)->name, extra_argv[0]);
-		args->extra = (*ar)->parse_extra(extra_argc, extra_argv);
+	if (compression_level != -1) {
+		if ((*ar)->flags & USES_ZLIB_COMPRESSION)
+			zlib_compression_level = compression_level;
+		else {
+			die("Argument not supported for format '%s': -%d",
+					format, compression_level);
+		}
 	}
 	args->verbose = verbose;
 	args->base = base;
-- 
1.5.6.2.212.g08b51

[PATCH 2/6] add context pointer to read_tree_recursive()

[René Scharfe]

Add a pointer parameter to read_tree_recursive(), which is passed to the
callback function.  This allows callers of read_tree_recursive() to
share data with the callback without resorting to global variables.  All
current callers pass NULL.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-tar.c             |   11 ++++++-----
 archive-zip.c             |   11 ++++++-----
 builtin-checkout.c        |    4 ++--
 builtin-log.c             |    4 ++--
 builtin-ls-tree.c         |    4 ++--
 builtin-merge-recursive.c |    4 ++--
 tree.c                    |   12 ++++++------
 tree.h                    |    4 ++--
 8 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 99db58f..6eaf59e 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -234,9 +234,9 @@ static int git_tar_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static int write_tar_entry(const unsigned char *sha1,
-                           const char *base, int baselen,
-                           const char *filename, unsigned mode, int stage)
+static int write_tar_entry(const unsigned char *sha1, const char *base,
+		int baselen, const char *filename, unsigned mode, int stage,
+		void *context)
 {
 	static struct strbuf path = STRBUF_INIT;
 	void *buffer;
@@ -286,11 +286,12 @@ int write_tar_archive(struct archiver_args *args)
 
 		while (baselen > 0 && base[baselen - 1] == '/')
 			base[--baselen] = '\0';
-		write_tar_entry(args->tree->object.sha1, "", 0, base, 040777, 0);
+		write_tar_entry(args->tree->object.sha1, "", 0, base, 040777,
+				0, NULL);
 		free(base);
 	}
 	read_tree_recursive(args->tree, args->base, plen, 0,
-			    args->pathspec, write_tar_entry);
+			    args->pathspec, write_tar_entry, NULL);
 	write_trailer();
 
 	return 0;
diff --git a/archive-zip.c b/archive-zip.c
index 5742762..0d24f3f 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -152,9 +152,9 @@ static char *construct_path(const char *base, int baselen,
 	return path;
 }
 
-static int write_zip_entry(const unsigned char *sha1,
-                           const char *base, int baselen,
-                           const char *filename, unsigned mode, int stage)
+static int write_zip_entry(const unsigned char *sha1, const char *base,
+		int baselen, const char *filename, unsigned mode, int stage,
+		void *context)
 {
 	struct zip_local_header header;
 	struct zip_dir_header dirent;
@@ -332,11 +332,12 @@ int write_zip_archive(struct archiver_args *args)
 
 		while (baselen > 0 && base[baselen - 1] == '/')
 			base[--baselen] = '\0';
-		write_zip_entry(args->tree->object.sha1, "", 0, base, 040777, 0);
+		write_zip_entry(args->tree->object.sha1, "", 0, base, 040777,
+				0, NULL);
 		free(base);
 	}
 	read_tree_recursive(args->tree, args->base, plen, 0,
-			    args->pathspec, write_zip_entry);
+			    args->pathspec, write_zip_entry, NULL);
 	write_zip_trailer(args->commit_sha1);
 
 	free(zip_dir);
diff --git a/builtin-checkout.c b/builtin-checkout.c
index d6641c2..a542033 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -43,7 +43,7 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
 }
 
 static int update_some(const unsigned char *sha1, const char *base, int baselen,
-		       const char *pathname, unsigned mode, int stage)
+		const char *pathname, unsigned mode, int stage, void * context)
 {
 	int len;
 	struct cache_entry *ce;
@@ -67,7 +67,7 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,
 
 static int read_tree_some(struct tree *tree, const char **pathspec)
 {
-	read_tree_recursive(tree, "", 0, 0, pathspec, update_some);
+	read_tree_recursive(tree, "", 0, 0, pathspec, update_some, NULL);
 
 	/* update the index with the given tree's info
 	 * for all args, expanding wildcards, and exit
diff --git a/builtin-log.c b/builtin-log.c
index 430d876..617aa67 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -313,7 +313,7 @@ static int show_object(const unsigned char *sha1, int show_tag_object,
 
 static int show_tree_object(const unsigned char *sha1,
 		const char *base, int baselen,
-		const char *pathname, unsigned mode, int stage)
+		const char *pathname, unsigned mode, int stage, void *context)
 {
 	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
 	return 0;
@@ -366,7 +366,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 					name,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, NULL,
-					show_tree_object);
+					show_tree_object, NULL);
 			break;
 		case OBJ_COMMIT:
 			rev.pending.nr = rev.pending.alloc = 0;
diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index f4a75dd..0da047c 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c
@@ -56,7 +56,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 }
 
 static int show_tree(const unsigned char *sha1, const char *base, int baselen,
-		     const char *pathname, unsigned mode, int stage)
+		const char *pathname, unsigned mode, int stage, void *context)
 {
 	int retval = 0;
 	const char *type = blob_type;
@@ -189,7 +189,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-	read_tree_recursive(tree, "", 0, 0, pathspec, show_tree);
+	read_tree_recursive(tree, "", 0, 0, pathspec, show_tree, NULL);
 
 	return 0;
 }
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 43bf6aa..385e742 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -256,7 +256,7 @@ struct tree *write_tree_from_memory(void)
 
 static int save_files_dirs(const unsigned char *sha1,
 		const char *base, int baselen, const char *path,
-		unsigned int mode, int stage)
+		unsigned int mode, int stage, void *context)
 {
 	int len = strlen(path);
 	char *newpath = xmalloc(baselen + len + 1);
@@ -276,7 +276,7 @@ static int save_files_dirs(const unsigned char *sha1,
 static int get_files_dirs(struct tree *tree)
 {
 	int n;
-	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs) != 0)
+	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, NULL))
 		return 0;
 	n = current_file_set.nr + current_directory_set.nr;
 	return n;
diff --git a/tree.c b/tree.c
index 4b1825c..03e782a 100644
--- a/tree.c
+++ b/tree.c
@@ -29,7 +29,7 @@ static int read_one_entry_opt(const unsigned char *sha1, const char *base, int b
 	return add_cache_entry(ce, opt);
 }
 
-static int read_one_entry(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage)
+static int read_one_entry(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage, void *context)
 {
 	return read_one_entry_opt(sha1, base, baselen, pathname, mode, stage,
 				  ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK);
@@ -39,7 +39,7 @@ static int read_one_entry(const unsigned char *sha1, const char *base, int basel
  * This is used when the caller knows there is no existing entries at
  * the stage that will conflict with the entry being added.
  */
-static int read_one_entry_quick(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage)
+static int read_one_entry_quick(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage, void *context)
 {
 	return read_one_entry_opt(sha1, base, baselen, pathname, mode, stage,
 				  ADD_CACHE_JUST_APPEND);
@@ -92,7 +92,7 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns
 int read_tree_recursive(struct tree *tree,
 			const char *base, int baselen,
 			int stage, const char **match,
-			read_tree_fn_t fn)
+			read_tree_fn_t fn, void *context)
 {
 	struct tree_desc desc;
 	struct name_entry entry;
@@ -106,7 +106,7 @@ int read_tree_recursive(struct tree *tree,
 		if (!match_tree_entry(base, baselen, entry.path, entry.mode, match))
 			continue;
 
-		switch (fn(entry.sha1, base, baselen, entry.path, entry.mode, stage)) {
+		switch (fn(entry.sha1, base, baselen, entry.path, entry.mode, stage, context)) {
 		case 0:
 			continue;
 		case READ_TREE_RECURSIVE:
@@ -126,7 +126,7 @@ int read_tree_recursive(struct tree *tree,
 			retval = read_tree_recursive(lookup_tree(entry.sha1),
 						     newbase,
 						     baselen + pathlen + 1,
-						     stage, match, fn);
+						     stage, match, fn, context);
 			free(newbase);
 			if (retval)
 				return -1;
@@ -174,7 +174,7 @@ int read_tree(struct tree *tree, int stage, const char **match)
 
 	if (!fn)
 		fn = read_one_entry_quick;
-	err = read_tree_recursive(tree, "", 0, stage, match, fn);
+	err = read_tree_recursive(tree, "", 0, stage, match, fn, NULL);
 	if (fn == read_one_entry || err)
 		return err;
 
diff --git a/tree.h b/tree.h
index dd25c53..2ff01a4 100644
--- a/tree.h
+++ b/tree.h
@@ -21,12 +21,12 @@ int parse_tree(struct tree *tree);
 struct tree *parse_tree_indirect(const unsigned char *sha1);
 
 #define READ_TREE_RECURSIVE 1
-typedef int (*read_tree_fn_t)(const unsigned char *, const char *, int, const char *, unsigned int, int);
+typedef int (*read_tree_fn_t)(const unsigned char *, const char *, int, const char *, unsigned int, int, void *);
 
 extern int read_tree_recursive(struct tree *tree,
 			       const char *base, int baselen,
 			       int stage, const char **match,
-			       read_tree_fn_t fn);
+			       read_tree_fn_t fn, void *context);
 
 extern int read_tree(struct tree *tree, int stage, const char **paths);
 
-- 
1.5.6.2.212.g08b51

[PATCH 4/6] archive: centralize archive entry writing

[René Scharfe]

Add the exported function write_archive_entries() to archive.c, which uses
the new ability of read_tree_recursive() to pass a context pointer to its
callback in order to centralize previously duplicated code.

The new callback function write_archive_entry() does the work that every
archiver backend needs to do: loading file contents, entering subdirectories,
handling file attributes, constructing the full path of the entry.  All that
done, it calls the backend specific write_archive_entry_fn_t function.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive.c     |   77 ++++++++++++++++++++++++++++++++++++++
 archive.h     |    4 ++
 archive-tar.c |  116 ++++++++++++++++++--------------------------------------
 archive-zip.c |   90 +++++++-------------------------------------
 4 files changed, 133 insertions(+), 154 deletions(-)

diff --git a/archive.c b/archive.c
index 6502b76..58de55e 100644
--- a/archive.c
+++ b/archive.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "commit.h"
 #include "attr.h"
+#include "archive.h"
 
 static void format_subst(const struct commit *commit,
                          const char *src, size_t len,
@@ -95,3 +96,79 @@ int is_archive_path_ignored(const char *path)
 		return 0;
 	return ATTR_TRUE(check[0].value);
 }
+
+struct archiver_context {
+	struct archiver_args *args;
+	write_archive_entry_fn_t write_entry;
+};
+
+static int write_archive_entry(const unsigned char *sha1, const char *base,
+		int baselen, const char *filename, unsigned mode, int stage,
+		void *context)
+{
+	static struct strbuf path = STRBUF_INIT;
+	struct archiver_context *c = context;
+	struct archiver_args *args = c->args;
+	write_archive_entry_fn_t write_entry = c->write_entry;
+	int err;
+	enum object_type type;
+	unsigned long size;
+	void *buffer;
+
+	strbuf_reset(&path);
+	strbuf_grow(&path, PATH_MAX);
+	strbuf_add(&path, base, baselen);
+	strbuf_addstr(&path, filename);
+
+	if (is_archive_path_ignored(path.buf + args->baselen))
+		return 0;
+
+	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
+		strbuf_addch(&path, '/');
+		if (args->verbose)
+			fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
+		err = write_entry(args, sha1, path.buf, path.len, mode, NULL, 0);
+		if (err)
+			return err;
+		return READ_TREE_RECURSIVE;
+	}
+
+	buffer = sha1_file_to_archive(path.buf + args->baselen, sha1, mode,
+			&type, &size, args->commit);
+	if (!buffer)
+		return error("cannot read %s", sha1_to_hex(sha1));
+	if (args->verbose)
+		fprintf(stderr, "%.*s\n", (int)path.len, path.buf);
+	err = write_entry(args, sha1, path.buf, path.len, mode, buffer, size);
+	free(buffer);
+	return err;
+}
+
+int write_archive_entries(struct archiver_args *args,
+		write_archive_entry_fn_t write_entry)
+{
+	struct archiver_context context;
+	int err;
+
+	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
+		size_t len = args->baselen;
+
+		while (len > 1 && args->base[len - 2] == '/')
+			len--;
+		if (args->verbose)
+			fprintf(stderr, "%.*s\n", (int)len, args->base);
+		err = write_entry(args, args->tree->object.sha1, args->base,
+				len, 040777, NULL, 0);
+		if (err)
+			return err;
+	}
+
+	context.args = args;
+	context.write_entry = write_entry;
+
+	err =  read_tree_recursive(args->tree, args->base, args->baselen, 0,
+			args->pathspec, write_archive_entry, &context);
+	if (err == READ_TREE_RECURSIVE)
+		err = 0;
+	return err;
+}
diff --git a/archive.h b/archive.h
index 34151f4..4e44549 100644
--- a/archive.h
+++ b/archive.h
@@ -20,6 +20,8 @@ typedef int (*write_archive_fn_t)(struct archiver_args *);
 
 typedef void *(*parse_extra_args_fn_t)(int argc, const char **argv);
 
+typedef int (*write_archive_entry_fn_t)(struct archiver_args *args, const unsigned char *sha1, const char *path, size_t pathlen, unsigned int mode, void *buffer, unsigned long size);
+
 struct archiver {
 	const char *name;
 	write_archive_fn_t write_archive;
@@ -44,4 +46,6 @@ 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);
 extern int is_archive_path_ignored(const char *path);
 
+extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
+
 #endif	/* ARCHIVE_H */
diff --git a/archive-tar.c b/archive-tar.c
index 63cc2ec..f9eb726 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -13,11 +13,7 @@
 static char block[BLOCKSIZE];
 static unsigned long offset;
 
-static time_t archive_time;
 static int tar_umask = 002;
-static int verbose;
-static const struct commit *commit;
-static size_t base_len;
 
 /* writes out the whole block, but only if it is full */
 static void write_if_needed(void)
@@ -114,22 +110,24 @@ static unsigned int ustar_header_chksum(const struct ustar_header *header)
 	return chksum;
 }
 
-static int get_path_prefix(const struct strbuf *path, int maxlen)
+static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
 {
-	int i = path->len;
+	size_t i = pathlen;
 	if (i > maxlen)
 		i = maxlen;
 	do {
 		i--;
-	} while (i > 0 && path->buf[i] != '/');
+	} while (i > 0 && path[i] != '/');
 	return i;
 }
 
-static void write_entry(const unsigned char *sha1, struct strbuf *path,
-                        unsigned int mode, void *buffer, unsigned long size)
+static int write_tar_entry(struct archiver_args *args,
+		const unsigned char *sha1, const char *path, size_t pathlen,
+		unsigned int mode, void *buffer, unsigned long size)
 {
 	struct ustar_header header;
 	struct strbuf ext_header;
+	int err = 0;
 
 	memset(&header, 0, sizeof(header));
 	strbuf_init(&ext_header, 0);
@@ -143,8 +141,6 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 		mode = 0100666;
 		sprintf(header.name, "%s.paxheader", sha1_to_hex(sha1));
 	} else {
-		if (verbose)
-			fprintf(stderr, "%.*s\n", (int)path->len, path->buf);
 		if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 			*header.typeflag = TYPEFLAG_DIR;
 			mode = (mode | 0777) & ~tar_umask;
@@ -155,24 +151,24 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 			*header.typeflag = TYPEFLAG_REG;
 			mode = (mode | ((mode & 0100) ? 0777 : 0666)) & ~tar_umask;
 		} else {
-			error("unsupported file mode: 0%o (SHA1: %s)",
-			      mode, sha1_to_hex(sha1));
-			return;
+			return error("unsupported file mode: 0%o (SHA1: %s)",
+					mode, sha1_to_hex(sha1));
 		}
-		if (path->len > sizeof(header.name)) {
-			int plen = get_path_prefix(path, sizeof(header.prefix));
-			int rest = path->len - plen - 1;
+		if (pathlen > sizeof(header.name)) {
+			size_t plen = get_path_prefix(path, pathlen,
+					sizeof(header.prefix));
+			size_t rest = pathlen - plen - 1;
 			if (plen > 0 && rest <= sizeof(header.name)) {
-				memcpy(header.prefix, path->buf, plen);
-				memcpy(header.name, path->buf + plen + 1, rest);
+				memcpy(header.prefix, path, plen);
+				memcpy(header.name, path + plen + 1, rest);
 			} else {
 				sprintf(header.name, "%s.data",
 				        sha1_to_hex(sha1));
 				strbuf_append_ext_header(&ext_header, "path",
-				                         path->buf, path->len);
+						path, pathlen);
 			}
 		} else
-			memcpy(header.name, path->buf, path->len);
+			memcpy(header.name, path, pathlen);
 	}
 
 	if (S_ISLNK(mode) && buffer) {
@@ -187,7 +183,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 
 	sprintf(header.mode, "%07o", mode & 07777);
 	sprintf(header.size, "%011lo", S_ISREG(mode) ? size : 0);
-	sprintf(header.mtime, "%011lo", archive_time);
+	sprintf(header.mtime, "%011lo", args->time);
 
 	sprintf(header.uid, "%07o", 0);
 	sprintf(header.gid, "%07o", 0);
@@ -202,22 +198,30 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,
 	sprintf(header.chksum, "%07o", ustar_header_chksum(&header));
 
 	if (ext_header.len > 0) {
-		write_entry(sha1, NULL, 0, ext_header.buf, ext_header.len);
+		err = write_tar_entry(args, sha1, NULL, 0, 0, ext_header.buf,
+				ext_header.len);
+		if (err)
+			return err;
 	}
 	strbuf_release(&ext_header);
 	write_blocked(&header, sizeof(header));
 	if (S_ISREG(mode) && buffer && size > 0)
 		write_blocked(buffer, size);
+	return err;
 }
 
-static void write_global_extended_header(const unsigned char *sha1)
+static int write_global_extended_header(struct archiver_args *args)
 {
+	const unsigned char *sha1 = args->commit_sha1;
 	struct strbuf ext_header;
+	int err;
 
 	strbuf_init(&ext_header, 0);
 	strbuf_append_ext_header(&ext_header, "comment", sha1_to_hex(sha1), 40);
-	write_entry(NULL, NULL, 0, ext_header.buf, ext_header.len);
+	err = write_tar_entry(args, NULL, NULL, 0, 0, ext_header.buf,
+			ext_header.len);
 	strbuf_release(&ext_header);
+	return err;
 }
 
 static int git_tar_config(const char *var, const char *value, void *cb)
@@ -234,63 +238,17 @@ static int git_tar_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static int write_tar_entry(const unsigned char *sha1, const char *base,
-		int baselen, const char *filename, unsigned mode, int stage,
-		void *context)
-{
-	static struct strbuf path = STRBUF_INIT;
-	void *buffer;
-	enum object_type type;
-	unsigned long size;
-
-	strbuf_reset(&path);
-	strbuf_grow(&path, PATH_MAX);
-	strbuf_add(&path, base, baselen);
-	strbuf_addstr(&path, filename);
-	if (is_archive_path_ignored(path.buf + base_len))
-		return 0;
-	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
-		strbuf_addch(&path, '/');
-		buffer = NULL;
-		size = 0;
-	} else {
-		buffer = sha1_file_to_archive(path.buf + base_len, sha1, mode,
-				&type, &size, commit);
-		if (!buffer)
-			die("cannot read %s", sha1_to_hex(sha1));
-	}
-
-	write_entry(sha1, &path, mode, buffer, size);
-	free(buffer);
-
-	return READ_TREE_RECURSIVE;
-}
-
 int write_tar_archive(struct archiver_args *args)
 {
-	git_config(git_tar_config, NULL);
+	int err = 0;
 
-	archive_time = args->time;
-	verbose = args->verbose;
-	commit = args->commit;
-	base_len = args->baselen;
+	git_config(git_tar_config, NULL);
 
 	if (args->commit_sha1)
-		write_global_extended_header(args->commit_sha1);
-
-	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
-		char *base = xstrdup(args->base);
-		int baselen = strlen(base);
-
-		while (baselen > 0 && base[baselen - 1] == '/')
-			base[--baselen] = '\0';
-		write_tar_entry(args->tree->object.sha1, "", 0, base, 040777,
-				0, NULL);
-		free(base);
-	}
-	read_tree_recursive(args->tree, args->base, args->baselen, 0,
-			    args->pathspec, write_tar_entry, NULL);
-	write_trailer();
-
-	return 0;
+		err = write_global_extended_header(args);
+	if (!err)
+		err = write_archive_entries(args, write_tar_entry);
+	if (!err)
+		write_trailer();
+	return err;
 }
diff --git a/archive-zip.c b/archive-zip.c
index d18254c..8131289 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -9,11 +9,8 @@
 #include "builtin.h"
 #include "archive.h"
 
-static int verbose;
 static int zip_date;
 static int zip_time;
-static const struct commit *commit;
-static size_t base_len;
 
 static unsigned char *zip_dir;
 static unsigned int zip_dir_size;
@@ -128,33 +125,9 @@ static void *zlib_deflate(void *data, unsigned long size,
 	return buffer;
 }
 
-static char *construct_path(const char *base, int baselen,
-                            const char *filename, int isdir, int *pathlen)
-{
-	int filenamelen = strlen(filename);
-	int len = baselen + filenamelen;
-	char *path, *p;
-
-	if (isdir)
-		len++;
-	p = path = xmalloc(len + 1);
-
-	memcpy(p, base, baselen);
-	p += baselen;
-	memcpy(p, filename, filenamelen);
-	p += filenamelen;
-	if (isdir)
-		*p++ = '/';
-	*p = '\0';
-
-	*pathlen = len;
-
-	return path;
-}
-
-static int write_zip_entry(const unsigned char *sha1, const char *base,
-		int baselen, const char *filename, unsigned mode, int stage,
-		void *context)
+static int write_zip_entry(struct archiver_args *args,
+		const unsigned char *sha1, const char *path, size_t pathlen,
+		unsigned int mode, void *buffer, unsigned long size)
 {
 	struct zip_local_header header;
 	struct zip_dir_header dirent;
@@ -163,33 +136,20 @@ static int write_zip_entry(const unsigned char *sha1, const char *base,
 	unsigned long uncompressed_size;
 	unsigned long crc;
 	unsigned long direntsize;
-	unsigned long size;
 	int method;
-	int result = -1;
-	int pathlen;
 	unsigned char *out;
-	char *path;
-	enum object_type type;
-	void *buffer = NULL;
 	void *deflated = NULL;
 
 	crc = crc32(0, NULL, 0);
 
-	path = construct_path(base, baselen, filename, S_ISDIR(mode), &pathlen);
-	if (is_archive_path_ignored(path + base_len))
-		return 0;
-	if (verbose)
-		fprintf(stderr, "%s\n", path);
 	if (pathlen > 0xffff) {
-		error("path too long (%d chars, SHA1: %s): %s", pathlen,
-		      sha1_to_hex(sha1), path);
-		goto out;
+		return error("path too long (%d chars, SHA1: %s): %s",
+				(int)pathlen, sha1_to_hex(sha1), path);
 	}
 
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 		method = 0;
 		attr2 = 16;
-		result = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 		out = NULL;
 		uncompressed_size = 0;
 		compressed_size = 0;
@@ -199,19 +159,13 @@ static int write_zip_entry(const unsigned char *sha1, const char *base,
 			(mode & 0111) ? ((mode) << 16) : 0;
 		if (S_ISREG(mode) && zlib_compression_level != 0)
 			method = 8;
-		result = 0;
-		buffer = sha1_file_to_archive(path + base_len, sha1, mode,
-				&type, &size, commit);
-		if (!buffer)
-			die("cannot read %s", sha1_to_hex(sha1));
 		crc = crc32(crc, buffer, size);
 		out = buffer;
 		uncompressed_size = size;
 		compressed_size = size;
 	} else {
-		error("unsupported file mode: 0%o (SHA1: %s)", mode,
-		      sha1_to_hex(sha1));
-		goto out;
+		return error("unsupported file mode: 0%o (SHA1: %s)", mode,
+				sha1_to_hex(sha1));
 	}
 
 	if (method == 8) {
@@ -278,12 +232,9 @@ static int write_zip_entry(const unsigned char *sha1, const char *base,
 		zip_offset += compressed_size;
 	}
 
-out:
-	free(buffer);
 	free(deflated);
-	free(path);
 
-	return result;
+	return 0;
 }
 
 static void write_zip_trailer(const unsigned char *sha1)
@@ -316,31 +267,20 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
 
 int write_zip_archive(struct archiver_args *args)
 {
+	int err;
+
 	dos_time(&args->time, &zip_date, &zip_time);
 
 	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
 	zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
-	verbose = args->verbose;
-	commit = args->commit;
-	base_len = args->baselen;
-
-	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
-		char *base = xstrdup(args->base);
-		int baselen = strlen(base);
-
-		while (baselen > 0 && base[baselen - 1] == '/')
-			base[--baselen] = '\0';
-		write_zip_entry(args->tree->object.sha1, "", 0, base, 040777,
-				0, NULL);
-		free(base);
-	}
-	read_tree_recursive(args->tree, args->base, args->baselen, 0,
-			    args->pathspec, write_zip_entry, NULL);
-	write_zip_trailer(args->commit_sha1);
+
+	err = write_archive_entries(args, write_zip_entry);
+	if (!err)
+		write_zip_trailer(args->commit_sha1);
 
 	free(zip_dir);
 
-	return 0;
+	return err;
 }
 
 void *parse_extra_zip_args(int argc, const char **argv)
-- 
1.5.6.2.212.g08b51

[PATCH 5/6] archive: unify file attribute handling

[René Scharfe]

Now that all file attribute handling for git archive has moved to archive.c,
we can unexport sha1_file_to_archive() and is_archive_path_ignored() even
disappears.

Add setup_archive_check(), modelled after similar functions used in the code
of other commands that support multiple file attributes.

Also remove convert_to_archive(), as it's only remaining function with
attribute handling gone was to call format_subst() if commit was not NULL,
which is now checked in sha1_file_to_archive().

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive.c |   63 ++++++++++++++++++++++--------------------------------------
 archive.h |    3 --
 2 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/archive.c b/archive.c
index 58de55e..b8b45ba 100644
--- a/archive.c
+++ b/archive.c
@@ -36,34 +36,9 @@ static void format_subst(const struct commit *commit,
 	free(to_free);
 }
 
-static int convert_to_archive(const char *path,
-                              const void *src, size_t len,
-                              struct strbuf *buf,
-                              const struct commit *commit)
-{
-	static struct git_attr *attr_export_subst;
-	struct git_attr_check check[1];
-
-	if (!commit)
-		return 0;
-
-	if (!attr_export_subst)
-		attr_export_subst = git_attr("export-subst", 12);
-
-	check[0].attr = attr_export_subst;
-	if (git_checkattr(path, ARRAY_SIZE(check), check))
-		return 0;
-	if (!ATTR_TRUE(check[0].value))
-		return 0;
-
-	format_subst(commit, src, len, buf);
-	return 1;
-}
-
-void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
-                           unsigned int mode, enum object_type *type,
-                           unsigned long *sizep,
-                           const struct commit *commit)
+static void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
+		unsigned int mode, enum object_type *type,
+		unsigned long *sizep, const struct commit *commit)
 {
 	void *buffer;
 
@@ -75,7 +50,8 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
 		strbuf_init(&buf, 0);
 		strbuf_attach(&buf, buffer, *sizep, *sizep + 1);
 		convert_to_working_tree(path, buf.buf, buf.len, &buf);
-		convert_to_archive(path, buf.buf, buf.len, &buf, commit);
+		if (commit)
+			format_subst(commit, buf.buf, buf.len, &buf);
 		buffer = strbuf_detach(&buf, &size);
 		*sizep = size;
 	}
@@ -83,18 +59,17 @@ void *sha1_file_to_archive(const char *path, const unsigned char *sha1,
 	return buffer;
 }
 
-int is_archive_path_ignored(const char *path)
+static void setup_archive_check(struct git_attr_check *check)
 {
 	static struct git_attr *attr_export_ignore;
-	struct git_attr_check check[1];
+	static struct git_attr *attr_export_subst;
 
-	if (!attr_export_ignore)
+	if (!attr_export_ignore) {
 		attr_export_ignore = git_attr("export-ignore", 13);
-
+		attr_export_subst = git_attr("export-subst", 12);
+	}
 	check[0].attr = attr_export_ignore;
-	if (git_checkattr(path, ARRAY_SIZE(check), check))
-		return 0;
-	return ATTR_TRUE(check[0].value);
+	check[1].attr = attr_export_subst;
 }
 
 struct archiver_context {
@@ -110,6 +85,9 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	struct archiver_context *c = context;
 	struct archiver_args *args = c->args;
 	write_archive_entry_fn_t write_entry = c->write_entry;
+	struct git_attr_check check[2];
+	const char *path_without_prefix;
+	int convert = 0;
 	int err;
 	enum object_type type;
 	unsigned long size;
@@ -119,9 +97,14 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	strbuf_grow(&path, PATH_MAX);
 	strbuf_add(&path, base, baselen);
 	strbuf_addstr(&path, filename);
+	path_without_prefix = path.buf + args->baselen;
 
-	if (is_archive_path_ignored(path.buf + args->baselen))
-		return 0;
+	setup_archive_check(check);
+	if (!git_checkattr(path_without_prefix, ARRAY_SIZE(check), check)) {
+		if (ATTR_TRUE(check[0].value))
+			return 0;
+		convert = ATTR_TRUE(check[1].value);
+	}
 
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 		strbuf_addch(&path, '/');
@@ -133,8 +116,8 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 		return READ_TREE_RECURSIVE;
 	}
 
-	buffer = sha1_file_to_archive(path.buf + args->baselen, sha1, mode,
-			&type, &size, args->commit);
+	buffer = sha1_file_to_archive(path_without_prefix, sha1, mode,
+			&type, &size, convert ? args->commit : NULL);
 	if (!buffer)
 		return error("cannot read %s", sha1_to_hex(sha1));
 	if (args->verbose)
diff --git a/archive.h b/archive.h
index 4e44549..88ee3be 100644
--- a/archive.h
+++ b/archive.h
@@ -43,9 +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);
-extern int is_archive_path_ignored(const char *path);
-
 extern int write_archive_entries(struct archiver_args *args, write_archive_entry_fn_t write_entry);
 
 #endif	/* ARCHIVE_H */
-- 
1.5.6.2.212.g08b51

[PATCH 3/6] archive: add baselen member to struct archiver_args

[René Scharfe]

Calculate the length of base and save it in a new member of struct
archiver_args.  This way we don't have to compute it in each of the
format backends.

Note: parse_archive_args() guarantees that ->base won't ever be NULL.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-tar.c     |    8 +++-----
 archive-zip.c     |    8 +++-----
 archive.h         |    1 +
 builtin-archive.c |    1 +
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 6eaf59e..63cc2ec 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -268,19 +268,17 @@ static int write_tar_entry(const unsigned char
*sha1, const char *base,

 int write_tar_archive(struct archiver_args *args)
 {
-	int plen = args->base ? strlen(args->base) : 0;
-
 	git_config(git_tar_config, NULL);

 	archive_time = args->time;
 	verbose = args->verbose;
 	commit = args->commit;
-	base_len = args->base ? strlen(args->base) : 0;
+	base_len = args->baselen;

 	if (args->commit_sha1)
 		write_global_extended_header(args->commit_sha1);

-	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
+	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
 		char *base = xstrdup(args->base);
 		int baselen = strlen(base);

@@ -290,7 +288,7 @@ int write_tar_archive(struct archiver_args *args)
 				0, NULL);
 		free(base);
 	}
-	read_tree_recursive(args->tree, args->base, plen, 0,
+	read_tree_recursive(args->tree, args->base, args->baselen, 0,
 			    args->pathspec, write_tar_entry, NULL);
 	write_trailer();

diff --git a/archive-zip.c b/archive-zip.c
index 0d24f3f..d18254c 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -316,17 +316,15 @@ static void dos_time(time_t *time, int *dos_date,
int *dos_time)

 int write_zip_archive(struct archiver_args *args)
 {
-	int plen = strlen(args->base);
-
 	dos_time(&args->time, &zip_date, &zip_time);

 	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
 	zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
 	verbose = args->verbose;
 	commit = args->commit;
-	base_len = args->base ? strlen(args->base) : 0;
+	base_len = args->baselen;

-	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
+	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
 		char *base = xstrdup(args->base);
 		int baselen = strlen(base);

@@ -336,7 +334,7 @@ int write_zip_archive(struct archiver_args *args)
 				0, NULL);
 		free(base);
 	}
-	read_tree_recursive(args->tree, args->base, plen, 0,
+	read_tree_recursive(args->tree, args->base, args->baselen, 0,
 			    args->pathspec, write_zip_entry, NULL);
 	write_zip_trailer(args->commit_sha1);

diff --git a/archive.h b/archive.h
index 1b24ae3..34151f4 100644
--- a/archive.h
+++ b/archive.h
@@ -6,6 +6,7 @@

 struct archiver_args {
 	const char *base;
+	size_t baselen;
 	struct tree *tree;
 	const unsigned char *commit_sha1;
 	const struct commit *commit;
diff --git a/builtin-archive.c b/builtin-archive.c
index 6ee3677..e7f4ec6 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -192,6 +192,7 @@ int parse_archive_args(int argc, const char **argv,
const struct archiver **ar,
 	}
 	args->verbose = verbose;
 	args->base = base;
+	args->baselen = strlen(base);

 	return i;
 }
-- 
1.5.6.2.212.g08b51

Re: [PATCH 3/6] archive: add baselen member to struct archiver_args

[René Scharfe]

[Resent with fixed line wrap.]
Calculate the length of base and save it in a new member of struct
archiver_args.  This way we don't have to compute it in each of the
format backends.

Note: parse_archive_args() guarantees that ->base won't ever be NULL.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-tar.c     |    8 +++-----
 archive-zip.c     |    8 +++-----
 archive.h         |    1 +
 builtin-archive.c |    1 +
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 6eaf59e..63cc2ec 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -268,19 +268,17 @@ static int write_tar_entry(const unsigned char *sha1, const char *base,
 
 int write_tar_archive(struct archiver_args *args)
 {
-	int plen = args->base ? strlen(args->base) : 0;
-
 	git_config(git_tar_config, NULL);
 
 	archive_time = args->time;
 	verbose = args->verbose;
 	commit = args->commit;
-	base_len = args->base ? strlen(args->base) : 0;
+	base_len = args->baselen;
 
 	if (args->commit_sha1)
 		write_global_extended_header(args->commit_sha1);
 
-	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
+	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
 		char *base = xstrdup(args->base);
 		int baselen = strlen(base);
 
@@ -290,7 +288,7 @@ int write_tar_archive(struct archiver_args *args)
 				0, NULL);
 		free(base);
 	}
-	read_tree_recursive(args->tree, args->base, plen, 0,
+	read_tree_recursive(args->tree, args->base, args->baselen, 0,
 			    args->pathspec, write_tar_entry, NULL);
 	write_trailer();
 
diff --git a/archive-zip.c b/archive-zip.c
index 0d24f3f..d18254c 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -316,17 +316,15 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
 
 int write_zip_archive(struct archiver_args *args)
 {
-	int plen = strlen(args->base);
-
 	dos_time(&args->time, &zip_date, &zip_time);
 
 	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
 	zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
 	verbose = args->verbose;
 	commit = args->commit;
-	base_len = args->base ? strlen(args->base) : 0;
+	base_len = args->baselen;
 
-	if (args->base && plen > 0 && args->base[plen - 1] == '/') {
+	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
 		char *base = xstrdup(args->base);
 		int baselen = strlen(base);
 
@@ -336,7 +334,7 @@ int write_zip_archive(struct archiver_args *args)
 				0, NULL);
 		free(base);
 	}
-	read_tree_recursive(args->tree, args->base, plen, 0,
+	read_tree_recursive(args->tree, args->base, args->baselen, 0,
 			    args->pathspec, write_zip_entry, NULL);
 	write_zip_trailer(args->commit_sha1);
 
diff --git a/archive.h b/archive.h
index 1b24ae3..34151f4 100644
--- a/archive.h
+++ b/archive.h
@@ -6,6 +6,7 @@
 
 struct archiver_args {
 	const char *base;
+	size_t baselen;
 	struct tree *tree;
 	const unsigned char *commit_sha1;
 	const struct commit *commit;
diff --git a/builtin-archive.c b/builtin-archive.c
index 6ee3677..e7f4ec6 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -192,6 +192,7 @@ int parse_archive_args(int argc, const char **argv, const struct archiver **ar,
 	}
 	args->verbose = verbose;
 	args->base = base;
+	args->baselen = strlen(base);
 
 	return i;
 }
-- 
1.5.6.2.212.g08b51