[PATCH 0/2] fix git-archive with empty trees

[PATCH 0/2] fix git-archive with empty trees

[Jeff King]

I noticed that "git archive" will barf when the root tree is empty.
Instead, it should probably return an empty archive. I doubt many people
really care about this corner case in practice, but it seems like we
should handle it more gracefully (and it's an easy fix).

It came to my attention because we track failed git-archive invocations
at GitHub, and a particular repo had a two commits: adding some content,
then reverting the original commit. You can also get there with "commit
--allow-empty" on a new branch (which is what the tests do).

I didn't bother even looking at empty subtrees. AFAIK, git should never
produce them (it omits the tree entirely if there is no content in it).
You would have to fake it using hash-object manually. I suspect it would
work just fine, as we already exercise the empty-dir code paths in the
tests I did add.

  [1/2]: test-lib: factor out $GIT_UNZIP setup
  [2/2]: archive: handle commits with an empty tree

-Peff

[PATCH 1/2] test-lib: factor out $GIT_UNZIP setup

[Jeff King]

We set up the $GIT_UNZIP variable and lazy prereq in
multiple places (and the next patch is about to add another
one). Let's factor it out to avoid repeating ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0024-crlf-archive.sh | 6 ------
 t/t5003-archive-zip.sh  | 6 ------
 t/test-lib.sh           | 6 ++++++
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index 5378787..4e9fa3c 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -3,12 +3,6 @@ test_description='respect crlf in git archive'
 test_description='respect crlf in git archive'
 
 . ./test-lib.sh
-GIT_UNZIP=${GIT_UNZIP:-unzip}
-
-test_lazy_prereq UNZIP '
-	"$GIT_UNZIP" -v
-	test $? -ne 127
-'
 
 test_expect_success setup '
 
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 7cfe9ca..0f5b42b 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -3,15 +3,9 @@ SUBSTFORMAT=%H%n
 test_description='git archive --format=zip test'
 
 . ./test-lib.sh
-GIT_UNZIP=${GIT_UNZIP:-unzip}
 
 SUBSTFORMAT=%H%n
 
-test_lazy_prereq UNZIP '
-	"$GIT_UNZIP" -v
-	test $? -ne 127
-'
-
 test_lazy_prereq UNZIP_SYMLINKS '
 	(
 		mkdir unzip-symlinks &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9e7f6b4..1f51025 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,3 +760,9 @@ test -w / || test_set_prereq SANITY
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY
+
+GIT_UNZIP=${GIT_UNZIP:-unzip}
+test_lazy_prereq UNZIP '
+	"$GIT_UNZIP" -v
+	test $? -ne 127
+'
-- 
1.8.2.rc3.4.gc6ed371

[PATCH 2/2] archive: handle commits with an empty tree

[Jeff King]

git-archive relies on get_pathspec to convert its argv into
a list of pathspecs. When get_pathspec is given an empty
argv list, it returns a single pathspec, the empty string,
to indicate that everything matches. When we feed this to
our path_exists function, we typically see that the pathspec
turns up at least one item in the tree, and we are happy.

But when our tree is empty, we erroneously think it is
because the pathspec is too limited, when in fact it is
simply that there is nothing to be found in the tree. This
is a weird corner case, but the correct behavior is almost
certainly to produce an empty archive, not to exit with an
error.

This patch teaches git-archive to create empty archives when
there is no pathspec given (we continue to complain if a
pathspec is given, since it obviously is not matched). It
also confirms that the tar and zip writers produce sane
output in this instance.

Signed-off-by: Jeff King <peff@peff.net>
---
 archive.c                       |   2 +-
 t/t5004-archive-corner-cases.sh |  83 ++++++++++++++++++++++++++++++++++++++++
 t/t5004/empty.zip               | Bin 0 -> 62 bytes
 3 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100755 t/t5004-archive-corner-cases.sh
 create mode 100644 t/t5004/empty.zip

diff --git a/archive.c b/archive.c
index 93e00bb..d254fa5 100644
--- a/archive.c
+++ b/archive.c
@@ -234,7 +234,7 @@ static void parse_pathspec_arg(const char **pathspec,
 	ar_args->pathspec = pathspec = get_pathspec("", pathspec);
 	if (pathspec) {
 		while (*pathspec) {
-			if (!path_exists(ar_args->tree, *pathspec))
+			if (**pathspec && !path_exists(ar_args->tree, *pathspec))
 				die("path not found: %s", *pathspec);
 			pathspec++;
 		}
diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
new file mode 100755
index 0000000..395dd58
--- /dev/null
+++ b/t/t5004-archive-corner-cases.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='test corner cases of git-archive'
+. ./test-lib.sh
+
+test_expect_success 'create commit with empty tree' '
+	git commit --allow-empty -m foo
+'
+
+# Make a dir and clean it up afterwards
+make_dir() {
+	mkdir "$1" &&
+	test_when_finished "rm -rf '$1'"
+}
+
+# Check that the dir given in "$1" contains exactly the
+# set of paths given as arguments.
+check_dir() {
+	dir=$1; shift
+	{
+		echo "$dir" &&
+		for i in "$@"; do
+			echo "$dir/$i"
+		done
+	} | sort >expect &&
+	find "$dir" -print | sort >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'tar archive of empty tree is empty' '
+	git archive --format=tar HEAD >empty.tar &&
+	make_dir extract &&
+	"$TAR" xf empty.tar -C extract &&
+	check_dir extract
+'
+
+test_expect_success 'tar archive of empty tree with prefix' '
+	git archive --format=tar --prefix=foo/ HEAD >prefix.tar &&
+	make_dir extract &&
+	"$TAR" xf prefix.tar -C extract &&
+	check_dir extract foo
+'
+
+test_expect_success UNZIP 'zip archive of empty tree is empty' '
+	# Detect the exit code produced when our particular flavor of unzip
+	# sees an empty archive. Infozip will generate a warning and exit with
+	# code 1. But in the name of sanity, we do not expect other unzip
+	# implementations to do the same thing (it would be perfectly
+	# reasonable to exit 0, for example).
+	#
+	# This makes our test less rigorous on some platforms (unzip may not
+	# handle the empty repo at all, making our later check of its exit code
+	# a no-op). But we cannot do anything reasonable except skip the test
+	# on such platforms anyway, and this is the moral equivalent.
+	"$GIT_UNZIP" "$TEST_DIRECTORY"/t5004/empty.zip
+	expect_code=$?
+
+	git archive --format=zip HEAD >empty.zip &&
+	make_dir extract &&
+	(
+		cd extract &&
+		test_expect_code $expect_code "$GIT_UNZIP" ../empty.zip
+	) &&
+	check_dir extract
+'
+
+test_expect_success UNZIP 'zip archive of empty tree with prefix' '
+	# We do not have to play exit-code tricks here, because our
+	# result should not be empty; it has a directory in it.
+	git archive --format=zip --prefix=foo/ HEAD >prefix.zip &&
+	make_dir extract &&
+	(
+		cd extract &&
+		"$GIT_UNZIP" ../prefix.zip
+	) &&
+	check_dir extract foo
+'
+
+test_expect_success 'archive complains about pathspec on empty tree' '
+	test_must_fail git archive --format=tar HEAD -- foo >/dev/null
+'
+
+test_done
diff --git a/t/t5004/empty.zip b/t/t5004/empty.zip
new file mode 100644
index 0000000000000000000000000000000000000000..1a76bb600558dc94913a80076fe8dbdef13c1b15
GIT binary patch
literal 62
zcmWIWW@TeQ0~!oz$rh;wDW;amhRMmM$%ZCLW|oGQX-P%~24+b{=4q+M#^$N!DF&$k
D8w?B~

literal 0
HcmV?d00001

-- 
1.8.2.rc3.4.gc6ed371

Re: [PATCH 0/2] fix git-archive with empty trees

[Jeff King]

On Sun, Mar 10, 2013 at 09:31:24PM -0400, Jeff King wrote:

> I noticed that "git archive" will barf when the root tree is empty.
> [...]
> I didn't bother even looking at empty subtrees. AFAIK, git should never
> produce them (it omits the tree entirely if there is no content in it).
> You would have to fake it using hash-object manually. I suspect it would
> work just fine, as we already exercise the empty-dir code paths in the
> tests I did add.

Curious, I went ahead and tested this. It does indeed work as expected.
The following tests can be squashed into patch 2/2 if we want:

diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index 395dd58..cdb7d7a 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -80,4 +80,23 @@ test_expect_success 'archive complains about pathspec on empty tree' '
 	test_must_fail git archive --format=tar HEAD -- foo >/dev/null
 '
 
+test_expect_success 'create a commit with an empty subtree' '
+	empty_tree=$(git hash-object -t tree /dev/null) &&
+	root_tree=$(printf "040000 tree $empty_tree\tsub\n" | git mktree)
+'
+
+test_expect_success 'archive empty subtree with no pathspec' '
+	git archive --format=tar $root_tree >subtree-all.tar &&
+	make_dir extract &&
+	"$TAR" xf subtree-all.tar -C extract &&
+	check_dir extract sub
+'
+
+test_expect_success 'archive empty subtree by direct pathspec' '
+	git archive --format=tar $root_tree -- sub >subtree-path.tar &&
+	make_dir extract &&
+	"$TAR" xf subtree-path.tar -C extract &&
+	check_dir extract sub
+'
+
 test_done

Re: [PATCH 0/2] fix git-archive with empty trees

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Mar 10, 2013 at 09:31:24PM -0400, Jeff King wrote:
>
>> I noticed that "git archive" will barf when the root tree is empty.
>> [...]
>> I didn't bother even looking at empty subtrees. AFAIK, git should never
>> produce them (it omits the tree entirely if there is no content in it).
>> You would have to fake it using hash-object manually. I suspect it would
>> work just fine, as we already exercise the empty-dir code paths in the
>> tests I did add.
>
> Curious, I went ahead and tested this. It does indeed work as expected.

Interesting.

> The following tests can be squashed into patch 2/2 if we want:

Why not---will do.  Thanks.

> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
> index 395dd58..cdb7d7a 100755
> --- a/t/t5004-archive-corner-cases.sh
> +++ b/t/t5004-archive-corner-cases.sh
> @@ -80,4 +80,23 @@ test_expect_success 'archive complains about pathspec on empty tree' '
>  	test_must_fail git archive --format=tar HEAD -- foo >/dev/null
>  '
>  
> +test_expect_success 'create a commit with an empty subtree' '
> +	empty_tree=$(git hash-object -t tree /dev/null) &&
> +	root_tree=$(printf "040000 tree $empty_tree\tsub\n" | git mktree)
> +'
> +
> +test_expect_success 'archive empty subtree with no pathspec' '
> +	git archive --format=tar $root_tree >subtree-all.tar &&
> +	make_dir extract &&
> +	"$TAR" xf subtree-all.tar -C extract &&
> +	check_dir extract sub
> +'
> +
> +test_expect_success 'archive empty subtree by direct pathspec' '
> +	git archive --format=tar $root_tree -- sub >subtree-path.tar &&
> +	make_dir extract &&
> +	"$TAR" xf subtree-path.tar -C extract &&
> +	check_dir extract sub
> +'
> +
>  test_done