[PATCH 1/2] Add git-archive

git-archive is a command to make TAR and ZIP archives of a git tree.
It helps prevent a proliferation of git-{format}-tree commands.

Instead of directly calling git-{tar,zip}-tree command, it defines
a very simple API, that archiver should implement and register in
"git-archive.c". This API is made up by 2 functions whose prototype
is defined in "archive.h" file.

 - The first one is used to parse 'extra' parameters which have
   signification only for the specific archiver. That would allow
   different archive backends to have different kind of options.

 - The second one is used to ask to an archive backend to build
   the archive given some already resolved parameters.

The main reason for making this API is to avoid using
git-{tar,zip}-tree commands, hence making them useless. Maybe it's
time for them to die ?

It also implements remote operations by defining a very simple
protocol: it first sends the name of the specific uploader followed
the repository name (git-upload-tar git://example.org/repo.git).
Then it sends "arguments" key word followed by all options given
when invoking 'git-archive'.

The remote protocol is implemented in "git-archive.c" for client
side and is triggered by "--remote=<repo>" option. For example,
to fetch a TAR archive in a remote repo, you can issue:

$ git archive --format=tar --remote=git://xxx/yyy/zzz.git HEAD

We choose to not make a new command "git-fetch-archive" for example,
avoind one more GIT command which should be nice for users (less
commands to remember, keeps existing --remote option).

Signed-off-by: Franck Bui-Huu <redacted>
---
 .gitignore          |    1
 Makefile            |    3 -
 archive.h           |   43 ++++++++
 builtin-archive.c   |  262 +++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin-tar-tree.c  |   66 +++++++++++++
 builtin.h           |    1
 generate-cmdlist.sh |    1
 git.c               |    1
 8 files changed, 377 insertions(+), 1 deletions(-)

diff --git a/.gitignore b/.gitignore
index 78cb671..a3e7ca1 100644
--- a/.gitignore
+++ b/.gitignore

@@ -8,6 +8,7 @@ git-apply
 git-applymbox
 git-applypatch
 git-archimport
+git-archive
 git-bisect
 git-branch
 git-cat-file

diff --git a/Makefile b/Makefile
index 389daf7..51ed4dd 100644
--- a/Makefile
+++ b/Makefile

@@ -242,7 +242,7 @@ LIB_FILE=libgit.a
 XDIFF_LIB=xdiff/lib.a

 LIB_H = \
-	blob.h cache.h commit.h csum-file.h delta.h \
+	archive.h blob.h cache.h commit.h csum-file.h delta.h \
 	diff.h object.h pack.h para-walk.h pkt-line.h quote.h refs.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h

@@ -267,6 +267,7 @@ LIB_OBJS = \
 BUILTIN_OBJS = \
 	builtin-add.o \
 	builtin-apply.o \
+	builtin-archive.o \
 	builtin-cat-file.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \

diff --git a/archive.h b/archive.h
new file mode 100644
index 0000000..6c69953
--- /dev/null
+++ b/archive.h

@@ -0,0 +1,43 @@
+#ifndef ARCHIVE_H
+#define ARCHIVE_H
+
+typedef int (*write_archive_fn_t)(struct tree *tree,
+				  const unsigned char *commit_sha1,
+				  const char *prefix,
+				  time_t time,
+				  const char **pathspec);
+
+typedef int (*parse_extra_args_fn_t)(int argc,
+				     const char **argv,
+				     const char **reason);
+
+struct archiver_struct {
+	const char *name;
+	write_archive_fn_t write_archive;
+	parse_extra_args_fn_t parse_extra;
+	const char *remote;
+	const char *prefix;
+};
+
+extern struct archiver_struct archivers[];
+
+extern int parse_archive_args(int argc, const char **argv,
+			      struct archiver_struct **ar,
+			      const char **reason);
+
+extern int parse_treeish_arg(const char **argv,
+			     struct tree **tree,
+			     const unsigned char **commit_sha1,
+			     time_t *archive_time,
+			     const char *prefix,
+			     const char **reason);
+/*
+ *
+ */
+extern int write_tar_archive(struct tree *tree,
+			     const unsigned char *commit_sha1,
+			     const char *prefix,
+			     time_t time,
+			     const char **pathspec);
+
+#endif	/* ARCHIVE_H */

diff --git a/builtin-archive.c b/builtin-archive.c
new file mode 100644
index 0000000..9341813
--- /dev/null
+++ b/builtin-archive.c

@@ -0,0 +1,262 @@
+/*
+ * Copyright (c) 2006 Franck Bui-Huu
+ * Copyright (c) 2006 Rene Scharfe
+ */
+#include <time.h>
+#include "cache.h"
+#include "builtin.h"
+#include "archive.h"
+#include "commit.h"
+#include "tree-walk.h"
+#include "exec_cmd.h"
+#include "pkt-line.h"
+
+static const char archive_usage[] = \
+"git-archive --format=<fmt> [--prefix=<prefix>/] [-0|...|-9]

<tree-ish> [path...]";
+
+struct archiver_struct archivers[] = {
+	{ "tar", write_tar_archive },
+};
+
+static void concat_argv(int argc, const char **argv, char line[], int size)
+{
+	char *p;
+	int len, i;
+
+	p = line;
+	for (i = 1; i < argc; i++) {
+		/* server needn't these options */
+		if (!strncmp(argv[i], "--format=", 9) ||
+		    !strncmp(argv[i], "--remote=", 9))
+			continue;
+
+		len = strlen(argv[i]);
+		if (p + len + 1> line + size)
+			die("too many options");
+
+		strcpy(p, argv[i]);
+		p += len;
+		*p++ = ' ';
+	}
+	if (p > line)
+		p--;
+	*p = '\0';
+}
+
+static int run_remote_archiver(struct archiver_struct *ar, int argc,
+			       const char **argv)
+{
+	char *url, buf[1024];
+	pid_t pid;
+	int fd[2];
+	int len, rv;
+
+	sprintf(buf, "git-upload-%s", ar->name);
+
+	url = strdup(ar->remote);
+	pid = git_connect(fd, url, buf);
+	if (pid < 0)
+		return pid;
+
+	concat_argv(argc, argv, buf, sizeof(buf));
+
+	packet_write(fd[1], "arguments %s\n", buf);
+	packet_flush(fd[1]);
+
+	len = packet_read_line(fd[0], buf, sizeof(buf));
+	if (!len)
+		die("git-archive: expected ACK/NAK, got EOF");
+	if (buf[len-1] == '\n')
+		buf[--len] = 0;
+	if (strcmp(buf, "ACK")) {
+		if (len > 5 && !strncmp(buf, "NACK ", 5))
+			die("git-archive: NACK %s", buf + 5);
+		die("git-archive: protocol error");
+	}
+
+	len = packet_read_line(fd[0], buf, sizeof(buf));
+	if (len)
+		die("git-archive: expected a flush");
+
+	/* Now, start reading from fd[0] and spit it out to stdout */
+	rv = copy_fd(fd[0], 1);
+
+	close(fd[0]);
+	rv |= finish_connect(pid);
+
+	return !!rv;
+}
+
+static struct archiver_struct *get_archiver(const char *name)
+{
+	struct archiver_struct *ar = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(archivers); i++) {
+		if (!strcmp(name, archivers[i].name)) {
+			ar = &archivers[i];
+			break;
+		}
+	}
+	return ar;
+}
+
+int parse_treeish_arg(const char **argv, struct tree **tree,
+		      const unsigned char **commit_sha1,
+		      time_t *archive_time, const char *prefix,
+		      const char **reason)
+{
+	const char *name = argv[0];
+	struct commit *commit;
+	unsigned char sha1[20];
+	int rv = -1;
+
+	if (get_sha1(name, sha1)) {
+		*reason = "Not a valid object name";
+		goto out;
+	}
+
+	commit = lookup_commit_reference_gently(sha1, 1);
+	if (commit) {
+		*commit_sha1 = commit->object.sha1;
+		*archive_time = commit->date;
+	} else {
+		*archive_time = time(NULL);
+	}
+
+	*tree = parse_tree_indirect(sha1);
+	if (*tree == NULL) {
+		*reason = "not a tree object";
+		goto out;
+	}
+
+	if (prefix) {
+		unsigned char tree_sha1[20];
+		unsigned int mode;
+		int err;
+
+		err = get_tree_entry((*tree)->object.sha1, prefix,
+				     tree_sha1, &mode);
+		if (err || !S_ISDIR(mode)) {
+			*reason = "current working directory is untracked";
+			goto out;
+		}
+		free(*tree);
+		*tree = parse_tree_indirect(tree_sha1);
+	}
+	//free(*tree);
+	rv = 0;
+out:
+	return rv;
+}
+
+int parse_archive_args(int argc, const char **argv,
+		       struct archiver_struct **ar,
+		       const char **reason)
+{
+	const char *format = NULL;
+	const char *remote = NULL;
+	const char *prefix = NULL;
+	int list = 0;
+	int rv = -1, i;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (!strcmp(arg, "--list") || !strcmp(arg, "-l")) {
+			list = 1;
+			continue;
+		}
+		if (!strncmp(arg, "--format=", 9)) {
+			format = arg + 9;
+			continue;
+		}
+		if (!strncmp(arg, "--prefix=", 9)) {
+			prefix = arg + 9;
+			continue;
+		}
+		if (!strncmp(arg, "--remote=", 9)) {
+			remote = arg + 9;
+			continue;
+		}
+		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
+			zlib_compression_level = arg[1] - '0';
+			continue;
+		}
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		if (arg[0] == '-') {
+			*reason = archive_usage;
+			goto out;
+		}
+		break;
+	}
+	if (list) {
+		if (!remote) {
+			int i;
+
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
+		}
+		*reason = "--list and --remote not supported together";
+		goto out;
+	}
+	if (argc - i < 1) {
+		*reason = archive_usage;
+		goto out;
+	}
+	if (!format){
+		*reason = "You must specify an archive format";
+		goto out;
+	}
+	*ar = get_archiver(format);
+	if (*ar == NULL) {
+		*reason = "Unknown archive format";
+		goto out;
+	}
+	if ((*ar)->parse_extra) {
+		if ((*ar)->parse_extra(argc, argv, reason) < 0)
+			goto out;
+	}
+	(*ar)->remote = remote;
+	(*ar)->prefix = prefix ? : "";
+	rv = i;
+out:
+	return rv;
+}
+
+int cmd_archive(int argc, const char **argv, const char *prefix)
+{
+	struct archiver_struct *ar;
+	const char *reason;
+	const char **pathspec;
+	struct tree *tree;
+	const unsigned char *commit_sha1;
+	time_t archive_time;
+	int rv;
+
+	rv = parse_archive_args(argc, argv, &ar, &reason);
+	if (rv < 0)
+		goto err;
+
+	if (ar->remote)
+		return run_remote_archiver(ar, argc, argv);
+
+	if (prefix == NULL)
+		prefix = setup_git_directory();
+
+	argv += rv;
+	if (parse_treeish_arg(argv, &tree, &commit_sha1,
+			      &archive_time, prefix, &reason) < 0)
+		goto err;
+
+	pathspec = get_pathspec(ar->prefix, argv + 1);
+
+	return ar->write_archive(tree, commit_sha1, ar->prefix,
+				 archive_time, pathspec);
+err:
+	return error("%s", reason);
+}

diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
index 61a4135..e0da01e 100644
--- a/builtin-tar-tree.c
+++ b/builtin-tar-tree.c

@@ -9,6 +9,7 @@ #include "strbuf.h"
 #include "tar.h"
 #include "builtin.h"
 #include "pkt-line.h"
+#include "archive.h"

 #define RECORDSIZE	(512)
 #define BLOCKSIZE	(RECORDSIZE * 20)

@@ -338,6 +339,71 @@ static int generate_tar(int argc, const
 	return 0;
 }

+static int write_tar_entry(const unsigned char *sha1,
+                           const char *base, int baselen,
+                           const char *filename, unsigned mode, int stage)
+{
+	static struct strbuf path;
+	int filenamelen = strlen(filename);
+	void *buffer;
+	char type[20];
+	unsigned long size;
+
+	if (!path.alloc) {
+		path.buf = xmalloc(PATH_MAX);
+		path.alloc = PATH_MAX;
+		path.len = path.eof = 0;
+	}
+	if (path.alloc < baselen + filenamelen) {
+		free(path.buf);
+		path.buf = xmalloc(baselen + filenamelen);
+		path.alloc = baselen + filenamelen;
+	}
+	memcpy(path.buf, base, baselen);
+	memcpy(path.buf + baselen, filename, filenamelen);
+	path.len = baselen + filenamelen;
+	if (S_ISDIR(mode)) {
+		strbuf_append_string(&path, "/");
+		buffer = NULL;
+		size = 0;
+	} else {
+		buffer = read_sha1_file(sha1, type, &size);
+		if (!buffer)
+			die("cannot read %s", sha1_to_hex(sha1));
+	}
+
+	write_entry(sha1, &path, mode, buffer, size);
+
+	return READ_TREE_RECURSIVE;
+}
+
+int write_tar_archive(struct tree *tree, const unsigned char *commit_sha1,
+                      const char *prefix, time_t time, const char **pathspec)
+{
+	int plen = strlen(prefix);
+
+	git_config(git_tar_config);
+
+	archive_time = time;
+
+	if (commit_sha1)
+		write_global_extended_header(commit_sha1);
+
+	if (prefix && plen > 0 && prefix[plen - 1] == '/') {
+		char *base = strdup(prefix);
+		int baselen = strlen(base);
+
+		while (baselen > 0 && base[baselen - 1] == '/')
+			base[--baselen] = '\0';
+		write_tar_entry(tree->object.sha1, "", 0, base, 040777, 0);
+		free(base);
+	}
+	read_tree_recursive(tree, prefix, plen, 0, pathspec, write_tar_entry);
+	write_trailer();
+
+	return 0;
+}
+
 static const char *exec = "git-upload-tar";

 static int remote_tar(int argc, const char **argv)

diff --git a/builtin.h b/builtin.h
index 8472c79..2391afb 100644
--- a/builtin.h
+++ b/builtin.h

@@ -15,6 +15,7 @@ extern int write_tree(unsigned char *sha

 extern int cmd_add(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
+extern int cmd_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const

char *prefix);

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index ec1eda2..5450918 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh

@@ -12,6 +12,7 @@ struct cmdname_help common_cmds[] = {"
 sort <<\EOF |
 add
 apply
+archive
 bisect
 branch
 checkout

diff --git a/git.c b/git.c
index 82c8fee..c62c5cf 100644
--- a/git.c
+++ b/git.c

@@ -218,6 +218,7 @@ static void handle_internal_command(int
 	} commands[] = {
 		{ "add", cmd_add, RUN_SETUP },
 		{ "apply", cmd_apply },
+		{ "archive", cmd_archive },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },

-- 
1.4.2.gbba4

"Franck Bui-Huu" [off-list ref] writes:

git-archive is a command to make TAR and ZIP archives of a git tree.
It helps prevent a proliferation of git-{format}-tree commands.

Thanks.  I like the overall structure, at least mostly.
Also dropping -tree suffix from the command name is nice, short
and sweet.

Obviously I cannot apply this patch because it is totally
whitespace damaged, but here are some comments.

The type of the first argument might have to be different,
depending on performance analysis by Rene on struct tree vs
struct tree_desc.

+typedef int (*parse_extra_args_fn_t)(int argc,
+				     const char **argv,
+				     const char **reason);
+

I do not see a way for parse_extra to record the parameter it
successfully parsed, other than in a source-file-global, static
variable.  Not a very nice design for a library, if we are
building one from scratch.

Also, you are passing "reason" around from everywhere, but that
is used by the caller to pass it to error(), so it might be
simpler to just call error() when you want to assign to *reason,
and make an error return.  The caller does not have to do
anything if you do that.  Your way might interact with the
remote protocol better, though -- I haven't thought this part
through yet so do not take this as a serious objection, but just
a comment.

+struct archiver_struct {
+	const char *name;
+	write_archive_fn_t write_archive;
+	parse_extra_args_fn_t parse_extra;
+	const char *remote;
+	const char *prefix;
+};

Somehow "struct foo_struct" makes me feel uneasy, when I do not
see the reason to call it "struct foo".

Also, the first three fields are permanent property of the
archiver while two are to wrap runtime arguments of one
particular invocation.  I would have liked...

+extern struct archiver_struct archivers[];

... this array to have only the former, and a separate structure
"struct archive_args" to be defined.

	struct archive_args {
        	const char *remote;
                const char *prefix;
	};

After parse_archive_args finds the archiver specified with
--format=*, it can call its parse_extra to retrieve a suitable
struct that has struct archive_args embedded at the beginning,
and then set remote and prefix on the returned structure.

Then a specific parse_extra implementation can be written like this:

	static struct tar_archive_args {
        	struct archive_args a;
                int z_compress;
                ...
	};

	struct archive_args *
        tar_archive_parse_extra(int ac, const char **av)
	{
        	struct tar_archive_args *args = xcalloc(1, sizeof(*args));

		while (ac--) {
			const char *arg = *++av;
                	if (arg[0] == '-' &&
                            '0' <= arg[1] && arg[1] <= '9')
				args->z_compress = arg[1] - '0';
			...
		}
		return (struct archive_args *)args;
	}

and this can be passed to tar_archive_write_archive as an
argument.

+extern int parse_treeish_arg(const char **argv,
+			     struct tree **tree,
+			     const unsigned char **commit_sha1,
+			     time_t *archive_time,
+			     const char *prefix,
+			     const char **reason);
+extern int write_tar_archive(struct tree *tree,
+			     const unsigned char *commit_sha1,
+			     const char *prefix,
+			     time_t time,
+			     const char **pathspec);

I suspect we would want "struct tree_desc" based interface,
instead of "struct tree".

+static const char archive_usage[] = \
+"git-archive --format=<fmt> [--prefix=<prefix>/] [-0|...|-9]
<tree-ish> [path...]";

I do not think "-[0-9]" belongs to generic "git-archive".  It
does not make much sense to run compress on zip output.  More
like:

git-archive --format=<fmt> [--prefix=<prefix>] [format specific options] <tree-ish> [path...]

It has one potential advantage, though -- git-daemon _could_
look at it and notice that the client asks for too expensive
compression level.  But I do not think it is the only way to
achive that to make "-[0-9]" a generic option.

+static int run_remote_archiver(struct archiver_struct *ar, int argc,
+			       const char **argv)
+{
+	char *url, buf[1024];
+	pid_t pid;
+	int fd[2];
+	int len, rv;
+
+	sprintf(buf, "git-upload-%s", ar->name);

Are you calling git-upload-{tar,zip,rar,...} here?

+	url = strdup(ar->remote);
+	pid = git_connect(fd, url, buf);
+	if (pid < 0)
+		return pid;
+
+	concat_argv(argc, argv, buf, sizeof(buf));
+	packet_write(fd[1], "arguments %s\n", buf);
+	packet_flush(fd[1]);

Parameter concatenation with SP is a bad idea for two reasons.
You cannot have SP in argument.  Also packet_write() may not
like the length of the arguments.

A sequence of one argument per packet, with prefix "argument "
for future extension so that we can send other stuff if/when
needed, followed by a flush would be preferred.

+	/* Now, start reading from fd[0] and spit it out to stdout */
+	rv = copy_fd(fd[0], 1);
+	close(fd[0]);
+	rv |= finish_connect(pid);

It was painful to bolt progress indicator support onto original
upload-pack protocol, while making sure that older and newer
clients and servers interoperate with each other.  Since this is
a new protocol, we should start with the side-band support from
the beginning (see upload-pack and look for use_sideband).

Instead of sending the payload straight out, upload-archive side
would read from the underlying archiver, and send it with
one-byte prefix to say if it is a normal payload (band 1),
message to stderr used to show progress indicator and error
messages (band 2), or error exit situation (band 3).  The client
side here would receive the packetized data and do the reverse.

+int parse_treeish_arg(const char **argv, struct tree **tree,
+		      const unsigned char **commit_sha1,
+		      time_t *archive_time, const char *prefix,
+		      const char **reason)
+{
...
+	if (prefix) {
+		unsigned char tree_sha1[20];
+		unsigned int mode;
+		int err;
+
+		err = get_tree_entry((*tree)->object.sha1, prefix,
+				     tree_sha1, &mode);
+		if (err || !S_ISDIR(mode)) {
+			*reason = "current working directory is untracked";
+			goto out;
+		}
+		free(*tree);
+		*tree = parse_tree_indirect(tree_sha1);
+	}

I like the simplicity of just optionally sending one subtree (or
the whole thing), but I think this part would be made more
efficient if we go with "struct tree_desc" based interface.

Also I wonder how this interacts with the pathspec you take from
the command line.  Personally I think this single subtree
support is good enough and limiting with pathspec is not needed.

+int parse_archive_args(int argc, const char **argv,
+		       struct archiver_struct **ar,
+		       const char **reason)
+{
...
+		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
+			zlib_compression_level = arg[1] - '0';
+			continue;
+		}

Commented on this part already.

+	if (list) {
+		if (!remote) {
+			int i;
+
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
+		}

You do not need a different "i" that shadows the outer one here.

+	(*ar)->remote = remote;
+	(*ar)->prefix = prefix ? : "";

Please be nicer to other people by staying away from GNU
extension "A ? : B", especially when A is so simple.

Junio C Hamano wrote:

"Franck Bui-Huu" [off-list ref] writes:

quoted

git-archive is a command to make TAR and ZIP archives of a git tree.
It helps prevent a proliferation of git-{format}-tree commands.

Thanks.  I like the overall structure, at least mostly.
Also dropping -tree suffix from the command name is nice, short
and sweet.

great !

Obviously I cannot apply this patch because it is totally
whitespace damaged, but here are some comments.

(sigh), sorry for that.

quoted

diff --git a/archive.h b/archive.h
new file mode 100644
index 0000000..6c69953
--- /dev/null
+++ b/archive.h

@@ -0,0 +1,43 @@
+#ifndef ARCHIVE_H
+#define ARCHIVE_H
+
+typedef int (*write_archive_fn_t)(struct tree *tree,
+				  const unsigned char *commit_sha1,
+				  const char *prefix,
+				  time_t time,
+				  const char **pathspec);

The type of the first argument might have to be different,
depending on performance analysis by Rene on struct tree vs
struct tree_desc.

OK. We'll wait for Rene.

quoted

+typedef int (*parse_extra_args_fn_t)(int argc,
+				     const char **argv,
+				     const char **reason);
+

I do not see a way for parse_extra to record the parameter it
successfully parsed, other than in a source-file-global, static
variable.  Not a very nice design for a library, if we are
building one from scratch.

Interesting, could you explain why static variables are not nice ?

Also, you are passing "reason" around from everywhere, but that
is used by the caller to pass it to error(), so it might be
simpler to just call error() when you want to assign to *reason,
and make an error return.  The caller does not have to do
anything if you do that.  Your way might interact with the
remote protocol better, though -- I haven't thought this part
through yet so do not take this as a serious objection, but just
a comment.

You might have missed my second patch:

		"[PATCH 2/2] Add git-upload-archive"

Basically the server can also use 'reason' to report a failure
description during NACK. I find it more useful than the simple
"server sent EOF" error message.

quoted

+struct archiver_struct {
+	const char *name;
+	write_archive_fn_t write_archive;
+	parse_extra_args_fn_t parse_extra;
+	const char *remote;
+	const char *prefix;
+};

Somehow "struct foo_struct" makes me feel uneasy, when I do not
see the reason to call it "struct foo".

no strong feeling here. I'll call it "struct archiver". BTW there
are a couple of "struct foo_struct" in git source...

Also, the first three fields are permanent property of the
archiver while two are to wrap runtime arguments of one
particular invocation.  I would have liked...

'remote' case is not a generic argument that can be passed to
archiver backends. Remember, the archiver backends only do local
operation. They do not know about remote protocol which is part
of git-archive command. That's the reason why I think we shouldn't
make this field part of arguments structure. It completely change
the behaviour of git-archive when it is used.

quoted

+extern struct archiver_struct archivers[];

... this array to have only the former, and a separate structure
"struct archive_args" to be defined.

	struct archive_args {
        	const char *remote;
                const char *prefix;
	};

After parse_archive_args finds the archiver specified with
--format=*, it can call its parse_extra to retrieve a suitable
struct that has struct archive_args embedded at the beginning,
and then set remote and prefix on the returned structure.

One bad side is that we need to malloc this embedded structure.
Therefore we have to free this embedded structure somewhere. 

We could have the following structures in archive.h, but we need
to export all these archiver backend definitions.

	struct tar_archive_args {
		int z_compress;
	};

	struct tar_archive_args {
		[...]
	};

	struct archive_args {
		const char		*prefix;
		struct tree		*tree;
		const unsigned char	*commit_sha1;
		const char		*prefix;
		time_t			time;
		const char		**pathspec;
		union {
			struct tar_archive_args tar_args;
			struct zip_archive_args zip_args;
		} u;
	};

	struct archiver {
		const char *name;
		write_archive_fn_t write_archive;
		parse_extra_args_fn_t parse_extra;
		const char *remote;
	};

	typedef int (*write_archive_fn_t)(struct archive_args *archive_args);

Then a specific parse_extra implementation can be written like this:

	static struct tar_archive_args {
        	struct archive_args a;
                int z_compress;
                ...
	};

	struct archive_args *
        tar_archive_parse_extra(int ac, const char **av)
	{
        	struct tar_archive_args *args = xcalloc(1, sizeof(*args));

		while (ac--) {
			const char *arg = *++av;
                	if (arg[0] == '-' &&
                            '0' <= arg[1] && arg[1] <= '9')
				args->z_compress = arg[1] - '0';
			...
		}
		return (struct archive_args *)args;
	}

and this can be passed to tar_archive_write_archive as an
argument.

quoted

+extern int parse_treeish_arg(const char **argv,
+			     struct tree **tree,
+			     const unsigned char **commit_sha1,
+			     time_t *archive_time,
+			     const char *prefix,
+			     const char **reason);
+extern int write_tar_archive(struct tree *tree,
+			     const unsigned char *commit_sha1,
+			     const char *prefix,
+			     time_t time,
+			     const char **pathspec);

I suspect we would want "struct tree_desc" based interface,
instead of "struct tree".

quoted

+static const char archive_usage[] = \
+"git-archive --format=<fmt> [--prefix=<prefix>/] [-0|...|-9]
<tree-ish> [path...]";

I do not think "-[0-9]" belongs to generic "git-archive".  It
does not make much sense to run compress on zip output.  More
like:

I forgot to remove that.

git-archive --format=<fmt> [--prefix=<prefix>] [format specific options] <tree-ish> [path...]

I forgot to change that.

quoted

+static int run_remote_archiver(struct archiver_struct *ar, int argc,
+			       const char **argv)
+{
+	char *url, buf[1024];
+	pid_t pid;
+	int fd[2];
+	int len, rv;
+
+	sprintf(buf, "git-upload-%s", ar->name);

Are you calling git-upload-{tar,zip,rar,...} here?

yes. Actually git-upload-{tar,zip,...} commands are going to be
removed, but git-daemon know them as a daemon service. It will
map these services to the generic "git-upload-archive" command.
One benefit is that we could still disable TAR format and enable
TGZ one. Please take a look to the second patch that adds
git-upload-archive command.

quoted

+	url = strdup(ar->remote);
+	pid = git_connect(fd, url, buf);
+	if (pid < 0)
+		return pid;
+
+	concat_argv(argc, argv, buf, sizeof(buf));
+	packet_write(fd[1], "arguments %s\n", buf);
+	packet_flush(fd[1]);

Parameter concatenation with SP is a bad idea for two reasons.
You cannot have SP in argument.  Also packet_write() may not
like the length of the arguments.

A sequence of one argument per packet, with prefix "argument "
for future extension so that we can send other stuff if/when
needed, followed by a flush would be preferred.

Absolutely.

quoted

+	/* Now, start reading from fd[0] and spit it out to stdout */
+	rv = copy_fd(fd[0], 1);
+	close(fd[0]);
+	rv |= finish_connect(pid);

It was painful to bolt progress indicator support onto original
upload-pack protocol, while making sure that older and newer
clients and servers interoperate with each other.  Since this is
a new protocol, we should start with the side-band support from
the beginning (see upload-pack and look for use_sideband).

Instead of sending the payload straight out, upload-archive side
would read from the underlying archiver, and send it with
one-byte prefix to say if it is a normal payload (band 1),
message to stderr used to show progress indicator and error
messages (band 2), or error exit situation (band 3).  The client
side here would receive the packetized data and do the reverse.

OK, I'll take a look

quoted

+int parse_treeish_arg(const char **argv, struct tree **tree,
+		      const unsigned char **commit_sha1,
+		      time_t *archive_time, const char *prefix,
+		      const char **reason)
+{
...
+	if (prefix) {
+		unsigned char tree_sha1[20];
+		unsigned int mode;
+		int err;
+
+		err = get_tree_entry((*tree)->object.sha1, prefix,
+				     tree_sha1, &mode);
+		if (err || !S_ISDIR(mode)) {
+			*reason = "current working directory is untracked";
+			goto out;
+		}
+		free(*tree);
+		*tree = parse_tree_indirect(tree_sha1);
+	}

I like the simplicity of just optionally sending one subtree (or
the whole thing), but I think this part would be made more
efficient if we go with "struct tree_desc" based interface.

Also I wonder how this interacts with the pathspec you take from
the command line.  Personally I think this single subtree
support is good enough and limiting with pathspec is not needed.

As I said in a previous email, I'm new with git internals. I prefer
let that part to others who better have a better knowledge on the
subject. I'll dig into that later though...

quoted

+int parse_archive_args(int argc, const char **argv,
+		       struct archiver_struct **ar,
+		       const char **reason)
+{
...
+		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
+			zlib_compression_level = arg[1] - '0';
+			continue;
+		}

Commented on this part already.

OK

quoted

+	if (list) {
+		if (!remote) {
+			int i;
+
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
+		}

You do not need a different "i" that shadows the outer one here.

Yes.

quoted

+	(*ar)->remote = remote;
+	(*ar)->prefix = prefix ? : "";

Please be nicer to other people by staying away from GNU
extension "A ? : B", especially when A is so simple.

sure.

Thanks for your comments,

		Franck

Franck Bui-Huu schrieb:

Junio C Hamano wrote:

quoted

"Franck Bui-Huu" [off-list ref] writes:

quoted

git-archive is a command to make TAR and ZIP archives of a git tree.
It helps prevent a proliferation of git-{format}-tree commands.

Thanks.  I like the overall structure, at least mostly.
Also dropping -tree suffix from the command name is nice, short
and sweet.

great !

quoted

Obviously I cannot apply this patch because it is totally
whitespace damaged, but here are some comments.

(sigh), sorry for that.

quoted

quoted

diff --git a/archive.h b/archive.h
new file mode 100644
index 0000000..6c69953
--- /dev/null
+++ b/archive.h

@@ -0,0 +1,43 @@
+#ifndef ARCHIVE_H
+#define ARCHIVE_H
+
+typedef int (*write_archive_fn_t)(struct tree *tree,
+				  const unsigned char *commit_sha1,
+				  const char *prefix,
+				  time_t time,
+				  const char **pathspec);

The type of the first argument might have to be different,
depending on performance analysis by Rene on struct tree vs
struct tree_desc.

OK. We'll wait for Rene.

The performance difference I noticed was caused by a memleak; the speed
advantage of a struct tree_desc based traverser is significant if you
look only at the traversers' performance, but it is lost in the noise
of the "real" work that the payload function is doing (see my other
mail).

quoted

quoted

+static int run_remote_archiver(struct archiver_struct *ar, int argc,
+			       const char **argv)
+{
+	char *url, buf[1024];
+	pid_t pid;
+	int fd[2];
+	int len, rv;
+
+	sprintf(buf, "git-upload-%s", ar->name);

Are you calling git-upload-{tar,zip,rar,...} here?

yes. Actually git-upload-{tar,zip,...} commands are going to be
removed, but git-daemon know them as a daemon service. It will
map these services to the generic "git-upload-archive" command.
One benefit is that we could still disable TAR format and enable
TGZ one. Please take a look to the second patch that adds
git-upload-archive command.

I don't think git-daemon should need to care about specific
archivers.  Policy decisions, like disallowing certain archive types
or compression levels, should be made in git-upload-archive.  This
way all code regarding archive uploading is found in one place:
git-upload-archive.  We can keep git-upload-tar as a legacy
interface, but please use only git-upload-archive for the new stuff
(and not git-upload-zip etc.).

quoted

quoted

+int parse_treeish_arg(const char **argv, struct tree **tree,
+		      const unsigned char **commit_sha1,
+		      time_t *archive_time, const char *prefix,
+		      const char **reason)
+{
...
+	if (prefix) {
+		unsigned char tree_sha1[20];
+		unsigned int mode;
+		int err;
+
+		err = get_tree_entry((*tree)->object.sha1, prefix,
+				     tree_sha1, &mode);
+		if (err || !S_ISDIR(mode)) {
+			*reason = "current working directory is untracked";
+			goto out;
+		}
+		free(*tree);
+		*tree = parse_tree_indirect(tree_sha1);
+	}

I like the simplicity of just optionally sending one subtree (or
the whole thing), but I think this part would be made more
efficient if we go with "struct tree_desc" based interface.

Also I wonder how this interacts with the pathspec you take from
the command line.  Personally I think this single subtree
support is good enough and limiting with pathspec is not needed.

[Note: There's potential for confusion here because we have two
types of prefixes.  One is the present working directory inside the
git archive, the other is the one specified with --prefix=.  Here
we have the working directory kind of prefix.]

IMHO should work like in the following example, and the code above
cuts off the Documentation part:

   $ cd Documentation
   $ git-archive --format=tar --prefix=v1.0/ HEAD howto | tar tf -
   v1.0/howto/
   v1.0/howto/isolate-bugs-with-bisect.txt
   ...

I agree that simple subtree matching would be enough, at least for
now.

René

Franck Bui-Huu schrieb:

Here occurs the memory leak that I've been talking about.  buffer needs
to be free'd.

+
+    return READ_TREE_RECURSIVE;
+}

Rene Scharfe wrote:

IMHO should work like in the following example, and the code above
cuts off the Documentation part:

   $ cd Documentation
   $ git-archive --format=tar --prefix=v1.0/ HEAD howto | tar tf -
   v1.0/howto/
   v1.0/howto/isolate-bugs-with-bisect.txt
   ...

I agree that simple subtree matching would be enough, at least for
now.

What about

     $ git-archive --format=tar --prefix=v1.0/ HEAD:Documentation/howto

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Jakub Narebski schrieb:

Rene Scharfe wrote:

quoted

IMHO should work like in the following example, and the code above 
cuts off the Documentation part:

$ cd Documentation $ git-archive --format=tar --prefix=v1.0/ HEAD howto | tar tf - 
v1.0/howto/ 
v1.0/howto/isolate-bugs-with-bisect.txt ...

I agree that simple subtree matching would be enough, at least for 
now.

What about

$ git-archive --format=tar --prefix=v1.0/ HEAD:Documentation/howto

That is fine, too (cutting off Documentation/howto).

My comment above was about the piece of code that handles cd'ing around in
the repository.  git-tar-tree ignores the current working directory -- you
always get the full tree put into your tar file, and you have to do the
"trick" you mentioned if you want to archive only a subtree.  This is a bit
strange, so I think we should do it right from the start in git-archive.

René