threads / patch / 18782

patch, 8 partsarchive: add a failure test wrt .gitattributes misreading

Subject: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

## tl;dr

24 messages between Apr 8, 2009 and Apr 18, 2009. Diffs are folded; open one to read it.

replies: 23people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · lore

[PATCH 0/8] support "in-tree attributes" for git-archive

This series attempts to fix "git archive" sometimes looking into .gitattributes in working directory and may cause undesired effects.

Nguyễn Thái Ngọc Duy (8):
  archive: add shortcuts for --format and --prefix
This one is unrelated. Just a convenient patch.
  move prune_cache() to git lib
  archive: add a failure test wrt .gitattributes  misreading
  archive: add tests for directory selection
  attr: add GIT_ATTR_INDEX "direction"
  archive: use index instead of parsing tree directly
  archive: disregard .gitattributes on working directory
Main part, which converts archive.c to load index first, then export from there.
  archive: support creating archives from index
Well, show off patch ;-)
 Documentation/git-archive.txt |    4 +-
 archive.c                     |  106 +++++++++++++++++++++++++++++++----------
 archive.h                     |    1 +
 attr.c                        |    4 +-
 attr.h                        |    3 +-
 builtin-ls-files.c            |   29 +-----------
 cache.h                       |    2 +
 read-cache.c                  |   27 ++++++++++
 t/t0024-crlf-archive.sh       |   35 +++++++++++++-
 t/t5000-tar-tree.sh           |    6 ++-
 10 files changed, 158 insertions(+), 59 deletions(-)
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/8] archive: add shortcuts for --format and --prefix

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-archive.txt |    4 +++-
 archive.c                     |    4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)
Show changes to 2 files +5 −3

Documentation/git-archive.txt, archive.c

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index c1adf59..c9fff54 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -9,7 +9,7 @@ git-archive - Create an archive of files from a named tree
 SYNOPSIS
 --------
 [verse]
-'git archive' --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
+'git archive' [-f|--format=<fmt>] [--list] [-p|--prefix=<prefix>/] [<extra>]
 	      [--output=<file>]
 	      [--remote=<repo> [--exec=<git-upload-archive>]] <tree-ish>
 	      [path...]
@@ -33,6 +33,7 @@ comment.
 OPTIONS
 -------
 
+-f=<fmt>::
 --format=<fmt>::
 	Format of the resulting archive: 'tar' or 'zip'.  The default
 	is 'tar'.
@@ -45,6 +46,7 @@ OPTIONS
 --verbose::
 	Report progress to stderr.
 
+-p=<prefix>/::
 --prefix=<prefix>/::
 	Prepend <prefix>/ to each filename in the archive.
 
diff --git a/archive.c b/archive.c
index 96b62d4..e87fed7 100644
--- a/archive.c
+++ b/archive.c
@@ -260,8 +260,8 @@ static int parse_archive_args(int argc, const char **argv,
 	int list = 0;
 	struct option opts[] = {
 		OPT_GROUP(""),
-		OPT_STRING(0, "format", &format, "fmt", "archive format"),
-		OPT_STRING(0, "prefix", &base, "prefix",
+		OPT_STRING('f', "format", &format, "fmt", "archive format"),
+		OPT_STRING('p', "prefix", &base, "prefix",
 			"prepend prefix to each pathname in the archive"),
 		OPT_STRING(0, "output", &output, "file",
 			"write the archive to this file"),
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/8] move prune_cache() to git lib

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin-ls-files.c |   29 +----------------------------
 cache.h            |    2 ++
 read-cache.c       |   27 +++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 28 deletions(-)
Show changes to 3 files +30 −28

builtin-ls-files.c, cache.h, read-cache.c

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 88e2697..0151647 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -206,33 +206,6 @@ static void show_files(struct dir_struct *dir, const char *prefix)
 	}
 }
 
-/*
- * Prune the index to only contain stuff starting with "prefix"
- */
-static void prune_cache(const char *prefix)
-{
-	int pos = cache_name_pos(prefix, prefix_len);
-	unsigned int first, last;
-
-	if (pos < 0)
-		pos = -pos-1;
-	memmove(active_cache, active_cache + pos,
-		(active_nr - pos) * sizeof(struct cache_entry *));
-	active_nr -= pos;
-	first = 0;
-	last = active_nr;
-	while (last > first) {
-		int next = (last + first) >> 1;
-		struct cache_entry *ce = active_cache[next];
-		if (!strncmp(ce->name, prefix, prefix_len)) {
-			first = next+1;
-			continue;
-		}
-		last = next;
-	}
-	active_nr = last;
-}
-
 static const char *verify_pathspec(const char *prefix)
 {
 	const char **p, *n, *prev;
@@ -541,7 +514,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		show_cached = 1;
 
 	if (prefix)
-		prune_cache(prefix);
+		prune_cache(prefix, prefix_len);
 	if (with_tree) {
 		/*
 		 * Basic sanity check; show-stages and show-unmerged
diff --git a/cache.h b/cache.h
index ab1294d..54660a5 100644
--- a/cache.h
+++ b/cache.h
@@ -335,6 +335,7 @@ static inline void remove_name_hash(struct cache_entry *ce)
 #define ce_modified(ce, st, options) ie_modified(&the_index, (ce), (st), (options))
 #define cache_name_exists(name, namelen, igncase) index_name_exists(&the_index, (name), (namelen), (igncase))
 #define cache_name_is_other(name, namelen) index_name_is_other(&the_index, (name), (namelen))
+#define prune_cache(prefix, prefix_len) prune_index((prefix), (prefix_len), &the_index);
 #endif
 
 enum object_type {
@@ -455,6 +456,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
 extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
+extern void prune_index(const char *prefix, int prefix_len, struct index_state *istate);
 
 /* do stat comparison even if CE_VALID is true */
 #define CE_MATCH_IGNORE_VALID		01
diff --git a/read-cache.c b/read-cache.c
index 3f58711..74f889e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1706,3 +1706,30 @@ int index_name_is_other(const struct index_state *istate, const char *name,
 	}
 	return 1;
 }
+
+/*
+ * Prune the index to only contain stuff starting with "prefix"
+ */
+void prune_index(const char *prefix, int prefix_len, struct index_state *istate)
+{
+	int pos = index_name_pos(istate, prefix, prefix_len);
+	unsigned int first, last;
+
+	if (pos < 0)
+		pos = -pos-1;
+	memmove(istate->cache, istate->cache + pos,
+		(istate->cache_nr - pos) * sizeof(struct cache_entry *));
+	istate->cache_nr -= pos;
+	first = 0;
+	last = istate->cache_nr;
+	while (last > first) {
+		int next = (last + first) >> 1;
+		struct cache_entry *ce = istate->cache[next];
+		if (!strncmp(ce->name, prefix, prefix_len)) {
+			first = next+1;
+			continue;
+		}
+		last = next;
+	}
+	istate->cache_nr = last;
+}
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

"git archive" is used to create archives from a tree (or commit), so it should not consult any .gitattributes files on working directory.

It currently does, so this patch records the failure.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t0024-crlf-archive.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Show changes to t/t0024-crlf-archive.sh +7 −0
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index c7d0324..9c56e0e 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -43,4 +43,11 @@ test_expect_success UNZIP 'zip archive' '
 
 '
 
+test_expect_failure '.gitattributes in workdir should not be consulted' '
+	echo "sample -crlf" > .gitattributes &&
+	git archive --format=tar HEAD |
+	( mkdir untarred2 && cd untarred2 && "$TAR" -xf - )
+	test_cmp sample untarred2/sample
+'
+
 test_done
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 4/8] archive: add tests for directory selection

While in archive.c, it says "pathspec". It's not really pathspec. Make a few tests to catch this point.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 archive.c               |   17 +++++++++++++++++
 t/t0024-crlf-archive.sh |   30 +++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletions(-)
Show changes to 2 files +46 −1

archive.c, t/t0024-crlf-archive.sh

diff --git a/archive.c b/archive.c
index e87fed7..5b54d95 100644
--- a/archive.c
+++ b/archive.c
@@ -15,6 +15,8 @@ static char const * const archive_usage[] = {
 
 #define USES_ZLIB_COMPRESSION 1
 
+static int list_archive(struct archiver_args *args);
+
 static const struct archiver {
 	const char *name;
 	write_archive_fn_t write_archive;
@@ -22,8 +24,23 @@ static const struct archiver {
 } archivers[] = {
 	{ "tar", write_tar_archive },
 	{ "zip", write_zip_archive, USES_ZLIB_COMPRESSION },
+	{ "dump-file-list", list_archive },
 };
 
+static int list_archive_entry(struct archiver_args *args,
+		const unsigned char *sha1, const char *path, size_t pathlen,
+		unsigned int mode, void *buffer, unsigned long size)
+{
+	printf("%s\n", path);
+	return 0;
+}
+
+static int list_archive(struct archiver_args *args)
+{
+	write_archive_entries(args, list_archive_entry);
+	return 0;
+}
+
 static void format_subst(const struct commit *commit,
                          const char *src, size_t len,
                          struct strbuf *buf)
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index 9c56e0e..695ad5a 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -9,14 +9,42 @@ test_expect_success setup '
 
 	git config core.autocrlf true
 
+	mkdir sub &&
 	printf "CRLF line ending\r\nAnd another\r\n" > sample &&
-	git add sample &&
+	cp sample sub/sample &&
+	git add sample sub/sample &&
 
 	test_tick &&
 	git commit -m Initial
 
 '
 
+cat <<\EOF > expected
+sample
+sub/
+sub/sample
+EOF
+
+test_expect_success 'archive without subdir' '
+	git archive --format=dump-file-list HEAD > output &&
+	test_cmp output expected
+'
+
+cat <<\EOF > expected
+sub/
+sub/sample
+EOF
+
+test_expect_success 'archive with subdir' '
+	git archive --format=dump-file-list HEAD sub > output &&
+	test_cmp output expected
+'
+
+test_expect_success 'subdir is not pathspec' '
+	test "$(git archive --format=dump-file-list HEAD sub/)" = ""
+	test "$(git archive --format=dump-file-list HEAD s\*)" = ""
+'
+
 test_expect_success 'tar archive' '
 
 	git archive --format=tar HEAD |
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 5/8] attr: add GIT_ATTR_INDEX "direction"

This instructs attr mechanism not to look into working .gitattributes at all. Needed by tools that does not handle working directory, such as "git archive".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 attr.c |    4 +++-
 attr.h |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)
Show changes to 2 files +5 −2

attr.c, attr.h

diff --git a/attr.c b/attr.c
index 43259e5..37ca288 100644
--- a/attr.c
+++ b/attr.c
@@ -405,7 +405,7 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 		if (!res)
 			res = read_attr_from_file(path, macro_ok);
 	}
-	else {
+	else if (direction == GIT_ATTR_CHECKIN) {
 		res = read_attr_from_file(path, macro_ok);
 		if (!res)
 			/*
@@ -415,6 +415,8 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 			 */
 			res = read_attr_from_index(path, macro_ok);
 	}
+	else
+		res = read_attr_from_index(path, macro_ok);
 	if (!res)
 		res = xcalloc(1, sizeof(*res));
 	return res;
diff --git a/attr.h b/attr.h
index 3a2f4ec..69b5767 100644
--- a/attr.h
+++ b/attr.h
@@ -33,7 +33,8 @@ int git_checkattr(const char *path, int, struct git_attr_check *);
 
 enum git_attr_direction {
 	GIT_ATTR_CHECKIN,
-	GIT_ATTR_CHECKOUT
+	GIT_ATTR_CHECKOUT,
+	GIT_ATTR_INDEX,
 };
 void git_attr_set_direction(enum git_attr_direction, struct index_state *);
 
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 6/8] archive: use index instead of parsing tree directly

Currently:
 - "git archive" parses trees directly
 - attr mechanism does not support in-tree attributes

Making attr.c support in-tree attributes is quite a non-trivial task, IMHO. Instead this patch makes "git archive" read tree to index first, then start exporting from there.

One minor regression: "git archive" now will no longer generate directory entries, only files.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 archive.c               |   66 ++++++++++++++++++++++++++++++++---------------
 t/t0024-crlf-archive.sh |    2 -
 2 files changed, 45 insertions(+), 23 deletions(-)
Show changes to 2 files +45 −23

archive.c, t/t0024-crlf-archive.sh

diff --git a/archive.c b/archive.c
index 5b54d95..402f6d4 100644
--- a/archive.c
+++ b/archive.c
@@ -4,6 +4,8 @@
 #include "attr.h"
 #include "archive.h"
 #include "parse-options.h"
+#include "unpack-trees.h"
+#include "dir.h"
 
 static char const * const archive_usage[] = {
 	"git archive [options] <tree-ish> [path...]",
@@ -167,7 +169,9 @@ int write_archive_entries(struct archiver_args *args,
 		write_archive_entry_fn_t write_entry)
 {
 	struct archiver_context context;
-	int err;
+	struct unpack_trees_options opts;
+	struct tree_desc t;
+	int i, prefix_len, err;
 
 	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
 		size_t len = args->baselen;
@@ -185,11 +189,42 @@ int write_archive_entries(struct archiver_args *args,
 	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;
+	memset(&opts, 0, sizeof(opts));
+	opts.index_only = 1;
+	opts.head_idx = -1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.fn = oneway_merge;
+	init_tree_desc(&t, args->tree->buffer, args->tree->size);
+	if (unpack_trees(1, &t, &opts))
+		return -1;
+	prefix_len = common_prefix(args->pathspec);
+	if (prefix_len)
+		prune_cache(args->pathspec[0], prefix_len);
+
+	for (i = 0;i < active_nr;i++) {
+		struct cache_entry *ce = active_cache[i];
+		const char **path;
+		const char *str, *prefix;
+		int match = 0;
+
+		if (!args->pathspec || !args->pathspec[0] || !*args->pathspec[0])
+			match = 1;
+
+		/* It's not really pathspec, so match_pathspec can't be used */
+		for (path = args->pathspec;!match && *path;path++) {
+			for (str = ce->name, prefix = *path;*prefix && *str == *prefix; str++, prefix++)
+				;
+			if (!*prefix && *str == '/')
+				match = 1;
+		}
+
+		if (match)
+			write_archive_entry(ce->sha1, args->base, args->baselen,
+					    ce->name, ce->ce_mode, ce_stage(ce),
+					    &context);
+	}
+	return 0;
 }
 
 static const struct archiver *lookup_archiver(const char *name)
@@ -207,9 +242,10 @@ static const struct archiver *lookup_archiver(const char *name)
 }
 
 static void parse_pathspec_arg(const char **pathspec,
-		struct archiver_args *ar_args)
+			       struct archiver_args *ar_args,
+			       const char *prefix)
 {
-	ar_args->pathspec = get_pathspec(ar_args->base, pathspec);
+	ar_args->pathspec = get_pathspec(prefix, pathspec);
 }
 
 static void parse_treeish_arg(const char **argv,
@@ -238,18 +274,6 @@ static void parse_treeish_arg(const char **argv,
 	if (tree == NULL)
 		die("not a tree object");
 
-	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))
-			die("current working directory is untracked");
-
-		tree = parse_tree_indirect(tree_sha1);
-	}
 	ar_args->tree = tree;
 	ar_args->commit_sha1 = commit_sha1;
 	ar_args->commit = commit;
@@ -356,7 +380,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 		prefix = setup_git_directory();
 
 	parse_treeish_arg(argv, &args, prefix);
-	parse_pathspec_arg(argv + 1, &args);
+	parse_pathspec_arg(argv + 1, &args, prefix);
 
 	git_config(git_default_config, NULL);
 
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index 695ad5a..1cbe511 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -21,7 +21,6 @@ test_expect_success setup '
 
 cat <<\EOF > expected
 sample
-sub/
 sub/sample
 EOF
 
@@ -31,7 +30,6 @@ test_expect_success 'archive without subdir' '
 '
 
 cat <<\EOF > expected
-sub/
 sub/sample
 EOF
 
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 7/8] archive: disregard .gitattributes on working directory

Some tests in t500-tar-tree.sh depends on in-worktree .gitattributes. Fix them too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 We could make an option to keep old behaviour. But is it worth it?
 archive.c               |    1 +
 t/t0024-crlf-archive.sh |    2 +-
 t/t5000-tar-tree.sh     |    6 +++++-
 3 files changed, 7 insertions(+), 2 deletions(-)
Show changes to 3 files +7 −2

archive.c, t/t0024-crlf-archive.sh, t/t5000-tar-tree.sh

diff --git a/archive.c b/archive.c
index 402f6d4..1ce644e 100644
--- a/archive.c
+++ b/archive.c
@@ -138,6 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	path_without_prefix = path.buf + args->baselen;
 
 	setup_archive_check(check);
+	git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
 	if (!git_checkattr(path_without_prefix, ARRAY_SIZE(check), check)) {
 		if (ATTR_TRUE(check[0].value))
 			return 0;
diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index 1cbe511..bf531fc 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -69,7 +69,7 @@ test_expect_success UNZIP 'zip archive' '
 
 '
 
-test_expect_failure '.gitattributes in workdir should not be consulted' '
+test_expect_success '.gitattributes in workdir should not be consulted' '
 	echo "sample -crlf" > .gitattributes &&
 	git archive --format=tar HEAD |
 	( mkdir untarred2 && cd untarred2 && "$TAR" -xf - )
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 7641e0d..420e7ad 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -56,6 +56,7 @@ test_expect_success \
     'add files to repository' \
     'find a -type f | xargs git update-index --add &&
      find a -type l | xargs git update-index --add &&
+     git update-index --add .gitattributes &&
      treeid=`git write-tree` &&
      echo $treeid >treeid &&
      git update-ref HEAD $(TZ=GMT GIT_COMMITTER_DATE="2005-05-27 22:00:00" \
@@ -140,6 +141,8 @@ test_expect_success \
 test_expect_success \
     'create archives with substfiles' \
     'echo "substfile?" export-subst >a/.gitattributes &&
+     git add a/.gitattributes &&
+     git commit -q -m new &&
      git archive HEAD >f.tar &&
      git archive --prefix=prefix/ HEAD >g.tar &&
      rm a/.gitattributes'
@@ -165,7 +168,8 @@ test_expect_success \
      'git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
       >g/prefix/a/substfile1.expected &&
       test_cmp g/prefix/a/substfile1.expected g/prefix/a/substfile1 &&
-      test_cmp a/substfile2 g/prefix/a/substfile2
+      test_cmp a/substfile2 g/prefix/a/substfile2 &&
+      git reset HEAD^
 '
 
 test_expect_success \
-- 
1.6.2.2.602.g83ee9f
Nguyễn Thái Ngọc Duy· Apr 8, 2009, 10:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 8/8] archive: support creating archives from index

This is more or less for fun. Does anyone really want to create archives from index?

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 archive.c |   38 +++++++++++++++++++++++++-------------
 archive.h |    1 +
 2 files changed, 26 insertions(+), 13 deletions(-)
Show changes to 2 files +26 −13

archive.c, archive.h

diff --git a/archive.c b/archive.c
index 1ce644e..bb0a0f1 100644
--- a/archive.c
+++ b/archive.c
@@ -181,7 +181,7 @@ int write_archive_entries(struct archiver_args *args,
 			len--;
 		if (args->verbose)
 			fprintf(stderr, "%.*s\n", (int)len, args->base);
-		err = write_entry(args, args->tree->object.sha1, args->base,
+		err = write_entry(args, args->tree ? args->tree->object.sha1 : null_sha1, args->base,
 				len, 040777, NULL, 0);
 		if (err)
 			return err;
@@ -190,15 +190,19 @@ int write_archive_entries(struct archiver_args *args,
 	context.args = args;
 	context.write_entry = write_entry;
 
-	memset(&opts, 0, sizeof(opts));
-	opts.index_only = 1;
-	opts.head_idx = -1;
-	opts.src_index = &the_index;
-	opts.dst_index = &the_index;
-	opts.fn = oneway_merge;
-	init_tree_desc(&t, args->tree->buffer, args->tree->size);
-	if (unpack_trees(1, &t, &opts))
-		return -1;
+	if (args->tree) {
+		memset(&opts, 0, sizeof(opts));
+		opts.index_only = 1;
+		opts.head_idx = -1;
+		opts.src_index = &the_index;
+		opts.dst_index = &the_index;
+		opts.fn = oneway_merge;
+		init_tree_desc(&t, args->tree->buffer, args->tree->size);
+		if (unpack_trees(1, &t, &opts))
+			return -1;
+	}
+	else
+		read_cache();
 	prefix_len = common_prefix(args->pathspec);
 	if (prefix_len)
 		prune_cache(args->pathspec[0], prefix_len);
@@ -300,6 +304,7 @@ static int parse_archive_args(int argc, const char **argv,
 	int verbose = 0;
 	int i;
 	int list = 0;
+	int use_index = 0;
 	struct option opts[] = {
 		OPT_GROUP(""),
 		OPT_STRING('f', "format", &format, "fmt", "archive format"),
@@ -307,6 +312,8 @@ static int parse_archive_args(int argc, const char **argv,
 			"prepend prefix to each pathname in the archive"),
 		OPT_STRING(0, "output", &output, "file",
 			"write the archive to this file"),
+		OPT_BOOLEAN('i', "index", &use_index,
+			"generate from index instead"),
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -348,7 +355,7 @@ static int parse_archive_args(int argc, const char **argv,
 	}
 
 	/* We need at least one parameter -- tree-ish */
-	if (argc < 1)
+	if (argc < 1 && !use_index)
 		usage_with_options(archive_usage, opts);
 	*ar = lookup_archiver(format);
 	if (!*ar)
@@ -366,6 +373,7 @@ static int parse_archive_args(int argc, const char **argv,
 	args->verbose = verbose;
 	args->base = base;
 	args->baselen = strlen(base);
+	args->use_index = use_index;
 
 	return argc;
 }
@@ -376,12 +384,16 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	const struct archiver *ar = NULL;
 	struct archiver_args args;
 
+	memset(&args, 0, sizeof(args));
 	argc = parse_archive_args(argc, argv, &ar, &args);
 	if (setup_prefix && prefix == NULL)
 		prefix = setup_git_directory();
 
-	parse_treeish_arg(argv, &args, prefix);
-	parse_pathspec_arg(argv + 1, &args, prefix);
+	if (!args.use_index) {
+		parse_treeish_arg(argv, &args, prefix);
+		argv++;
+	}
+	parse_pathspec_arg(argv, &args, prefix);
 
 	git_config(git_default_config, NULL);
 
diff --git a/archive.h b/archive.h
index 0b15b35..fb94d47 100644
--- a/archive.h
+++ b/archive.h
@@ -10,6 +10,7 @@ struct archiver_args {
 	time_t time;
 	const char **pathspec;
 	unsigned int verbose : 1;
+	unsigned int use_index : 1;
 	int compression_level;
 };
 
-- 
1.6.2.2.602.g83ee9f
René Scharfe· Apr 8, 2009, 20:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 8/8] archive: support creating archives from index

Nguyễn Thái Ngọc Duy schrieb:
> This is more or less for fun. Does anyone really want to create archives
> from index?
Perhaps, but I think it would be better implemented similar to git grep.
René
Junio C Hamano· Apr 8, 2009, 19:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 7/8] archive: disregard .gitattributes on working directory

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> Some tests in t500-tar-tree.sh depends on in-worktree
> .gitattributes. Fix them too.
s/500/5000/;

You can also consider these as examples of possibly useful use cases of the current "fix-attributes" feature you are breaking with this patch (see my comments to 3/8).

Junio C Hamano· Apr 8, 2009, 19:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 6/8] archive: use index instead of parsing tree directly

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 11 quoted lines
> Currently:
>
>  - "git archive" parses trees directly
>  - attr mechanism does not support in-tree attributes
>
> Making attr.c support in-tree attributes is quite a non-trivial task,
> IMHO. Instead this patch makes "git archive" read tree to index first,
> then start exporting from there.
>
> One minor regression: "git archive" now will no longer generate
> directory entries, only files.

Hmmm, if you read_tree() into the_index upfront and do not change anything else to the archive.c code, shouldn't it work without such a regression at all? Am I missing something?

It would allow you to export the index into an archive, but I doubt it is worth the amount of code churn.

René Scharfe· Apr 8, 2009, 20:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 6/8] archive: use index instead of parsing tree directly

Nguyễn Thái Ngọc Duy schrieb:
Show 7 quoted lines
> +		/* It's not really pathspec, so match_pathspec can't be used */
> +		for (path = args->pathspec;!match && *path;path++) {
> +			for (str = ce->name, prefix = *path;*prefix && *str == *prefix; str++, prefix++)
> +				;
> +			if (!*prefix && *str == '/')
> +				match = 1;
> +		}
This works with, e.g.:
	git archive HEAD Documentation
but not with:
	git archive HEAD Makefile
or:
	git archive HEAD Documentation/
René
René Scharfe· Apr 8, 2009, 21:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 6/8] archive: use index instead of parsing tree directly

Nguyễn Thái Ngọc Duy schrieb:
Show 11 quoted lines
> Currently:
> 
>  - "git archive" parses trees directly
>  - attr mechanism does not support in-tree attributes
> 
> Making attr.c support in-tree attributes is quite a non-trivial task,
> IMHO. Instead this patch makes "git archive" read tree to index first,
> then start exporting from there.
> 
> One minor regression: "git archive" now will no longer generate
> directory entries, only files.

I don't like the need to prepare an index of all paths up front, but that's just a gut feeling. I haven't looked into implementing in-tree attribute support in attr.c; is it really that hard? Other commands would benefit from this, too, right (e.g. any command using attributes in a bare repo)?

René
René Scharfe· Apr 8, 2009, 19:52 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 4/8] archive: add tests for directory selection

Nguyễn Thái Ngọc Duy schrieb:
Show 36 quoted lines
> While in archive.c, it says "pathspec". It's not really pathspec.
> Make a few tests to catch this point.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  archive.c               |   17 +++++++++++++++++
>  t/t0024-crlf-archive.sh |   30 +++++++++++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletions(-)
> 
> diff --git a/archive.c b/archive.c
> index e87fed7..5b54d95 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -15,6 +15,8 @@ static char const * const archive_usage[] = {
>  
>  #define USES_ZLIB_COMPRESSION 1
>  
> +static int list_archive(struct archiver_args *args);
> +
>  static const struct archiver {
>  	const char *name;
>  	write_archive_fn_t write_archive;
> @@ -22,8 +24,23 @@ static const struct archiver {
>  } archivers[] = {
>  	{ "tar", write_tar_archive },
>  	{ "zip", write_zip_archive, USES_ZLIB_COMPRESSION },
> +	{ "dump-file-list", list_archive },
>  };
>  
> +static int list_archive_entry(struct archiver_args *args,
> +		const unsigned char *sha1, const char *path, size_t pathlen,
> +		unsigned int mode, void *buffer, unsigned long size)
> +{
> +	printf("%s\n", path);
> +	return 0;
> +}

git archive dumps all its output, so I think "file-list" would be a better name for an archiver creating lists of files.

However, if you only need it for tests (I can't imagine other uses at the moment), you could do something like this instead:

	$ git archive -v HEAD >/dev/null 2>file-list

It might be wasteful since it creates a full archive (including file contents) and throws it away, but it's OK for testing purposes.

René
Junio C Hamano· Apr 8, 2009, 19:20 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> "git archive" is used to create archives from a tree (or commit), so
> it should not consult any .gitattributes files on working directory.
>
> It currently does,...

...which _might_ be actively used as a _feature_ by people; I do not think it is a bad idea to have a mode of operation where it solely works with in-tree attributes ignoring the work tree ones, and I suspect we probably would want to make that the default, but "so it should not" is probably a bit too strong.

For example, an older version may record $Id$ with keyword expansion set in its in-tree .gitattributes, and you later found it to be a mistake and have removed it in the current version. Re-exporting an older version with the current code wouldn't have $Id$ expanded, but with your patch it will.

A stronger example would be crlf conversion, I suppose, but the same "with the current code we can fix it up but after this patch we can't" worry applies.

So perhaps you may want to resurrect the current behaviour with a new option --fix-attributes, whose description would be "normally the command takes attributes settings from the tree being exported, but .gitattributes files in it may record wrong attributes you may want to fix while exporting. With this option, corresponding .gitattributes files in the work tree will override the in-tree .gitattributes" or something like that.

René Scharfe· Apr 13, 2009, 13:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

Junio C Hamano schrieb:
Show 30 quoted lines
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
>> "git archive" is used to create archives from a tree (or commit), so
>> it should not consult any .gitattributes files on working directory.
>>
>> It currently does,...
> 
> ...which _might_ be actively used as a _feature_ by people; I do not think
> it is a bad idea to have a mode of operation where it solely works with
> in-tree attributes ignoring the work tree ones, and I suspect we probably
> would want to make that the default, but "so it should not" is probably a
> bit too strong.
> 
> For example, an older version may record $Id$ with keyword expansion set
> in its in-tree .gitattributes, and you later found it to be a mistake and
> have removed it in the current version.  Re-exporting an older version
> with the current code wouldn't have $Id$ expanded, but with your patch it
> will.
> 
> A stronger example would be crlf conversion, I suppose, but the same "with
> the current code we can fix it up but after this patch we can't" worry
> applies.
> 
> So perhaps you may want to resurrect the current behaviour with a new
> option --fix-attributes, whose description would be "normally the command
> takes attributes settings from the tree being exported, but .gitattributes
> files in it may record wrong attributes you may want to fix while
> exporting.  With this option, corresponding .gitattributes files in the
> work tree will override the in-tree .gitattributes" or something like
> that.

Hmm, "fix" implies that something is broken without this option, which is not necessarily the case. A purely descriptive name like --worktree-attributes fits better IMHO.

Also, the last sentence of the description may be taken to imply a mixing of attributes from work tree and exported tree is done, which is not the case with the patch from series 2 (and shouldn't be). Perhaps: "With this option, .gitattributes files are read from the work tree instead of from the exported tree."

René
Nguyen Thai Ngoc Duy· Apr 14, 2009, 06:41 UTC · re: René Scharfe · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

2009/4/13 René Scharfe <rene.scharfe@lsrfire.ath.cx>:
Show 5 quoted lines
> Also, the last sentence of the description may be taken to imply a
> mixing of attributes from work tree and exported tree is done, which is
> not the case with the patch from series 2 (and shouldn't be).  Perhaps:
> "With this option, .gitattributes files are read from the work tree
> instead of from the exported tree."

I'm at work and haven't had chance to read other mails carefully. Wrt to the description, it reads from index too if it fails to read from worktree. So perhaps "With this option, .gitattributes files are read from work tree (or from index if they are missing from work tree) instead of from exported tree.", unless you want to introduce another "attr direction" to read from work tree only.

-- 
Duy
René Scharfe· Apr 14, 2009, 20:12 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

Nguyen Thai Ngoc Duy schrieb:
Show 13 quoted lines
> 2009/4/13 René Scharfe <rene.scharfe@lsrfire.ath.cx>:
>> Also, the last sentence of the description may be taken to imply a
>> mixing of attributes from work tree and exported tree is done, which is
>> not the case with the patch from series 2 (and shouldn't be).  Perhaps:
>> "With this option, .gitattributes files are read from the work tree
>> instead of from the exported tree."
> 
> I'm at work and haven't had chance to read other mails carefully. Wrt
> to the description, it reads from index too if it fails to read from
> worktree. So perhaps "With this option, .gitattributes files are read
> from work tree (or from index if they are missing from work tree)
> instead of from exported tree.", unless you want to introduce another
> "attr direction" to read from work tree only.

Err, yes, that was nonsense, please disregard this part. Sorry. I somehow mixed up falling back to the index and merging the contents of attribute files from both sources, not sure why. :-/

René
Junio C Hamano· Apr 16, 2009, 08:22 UTC · re: René Scharfe · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> Hmm, "fix" implies that something is broken without this option, which
> is not necessarily the case.

"What is recorded in the tree is unsuitable for the purpose of this particular export, so we are xxxing using yyy", is the intended use case, and I think the verb "fix" is a good fit there in "xxx" part. "Tweak" would also work well.

Your suggestion is to say "attributes from the work tree" in place of "yyy", which also is good.

I got the impression that people tend to prefer the operation named after what it does than how it does it, but I can go either way.

René Scharfe· Apr 17, 2009, 20:15 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

Junio C Hamano schrieb:
Show 12 quoted lines
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> Hmm, "fix" implies that something is broken without this option, which
>> is not necessarily the case.
> 
> "What is recorded in the tree is unsuitable for the purpose of this
> particular export, so we are xxxing using yyy", is the intended use case,
> and I think the verb "fix" is a good fit there in "xxx" part.  "Tweak"
> would also work well.
> 
> Your suggestion is to say "attributes from the work tree" in place of
> "yyy", which also is good.
> I got the impression that people tend to prefer the operation named after
> what it does than how it does it, but I can go either way.

That's a good rule to follow in general. "Fix" sounds to me as if git's default behaviour is wrong and needs an option to make it work correctly, though. It's too broad a term.

I think the fixing, tweaking or even breaking will be done by the user, when editing the attribute files in the work tree. git then only applies or evaluates these attributes, creating the archive using a different source of meta data. "--worktree-attributes" captures this nicely.

René
Junio C Hamano· Apr 18, 2009, 01:33 UTC · re: René Scharfe · lore

Re: [PATCH 3/8] archive: add a failure test wrt .gitattributes misreading

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 5 quoted lines
> I think the fixing, tweaking or even breaking will be done by the user,
> when editing the attribute files in the work tree.  git then only
> applies or evaluates these attributes, creating the archive using a
> different source of meta data.  "--worktree-attributes" captures this
> nicely.

Yeah, that sounds like a sane idea. I see you already made it so with your v2 4/5.

Thanks.
Junio C Hamano· Apr 8, 2009, 19:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/8] move prune_cache() to git lib

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> +#define prune_cache(prefix, prefix_len) prune_index((prefix), (prefix_len), &the_index);
> ...
> +extern void prune_index(const char *prefix, int prefix_len, struct index_state *istate);

Please make *istate the first parameter to be consistent with other distim_cache() wrappers for distim_index().

René Scharfe· Apr 8, 2009, 19:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/8] archive: add shortcuts for --format and --prefix

Nguyễn Thái Ngọc Duy schrieb:
Show 11 quoted lines
> +-f=<fmt>::
>  --format=<fmt>::
>  	Format of the resulting archive: 'tar' or 'zip'.  The default
>  	is 'tar'.
> @@ -45,6 +46,7 @@ OPTIONS
>  --verbose::
>  	Report progress to stderr.
>  
> +-p=<prefix>/::
>  --prefix=<prefix>/::
>  	Prepend <prefix>/ to each filename in the archive.

With short options, you can't use an equal sign to separate their parameters (-f=<fmt> vs. -f<fmt>).

René

← back to recent threads