threads / patch / 33150

patch, 2 partsfix git-archive with empty trees

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

## tl;dr

5 messages between Mar 11, 2013 and Mar 11, 2013. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Mar 11, 2013, 01:31 UTC · lore

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
Jeff King· Mar 11, 2013, 01:31 UTC · re: Jeff King · lore

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

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(-)
Show changes to 3 files +6 −12

t/t0024-crlf-archive.sh, t/t5003-archive-zip.sh, t/test-lib.sh

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
Jeff King· Mar 11, 2013, 01:32 UTC · re: Jeff King · lore

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

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
Show changes to 3 files +84 −1

archive.c, t/t5004-archive-corner-cases.sh, 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
Jeff King· Mar 11, 2013, 04:58 UTC · re: Jeff King · lore

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

On Sun, Mar 10, 2013 at 09:31:24PM -0400, Jeff King wrote:
Show 7 quoted lines
> 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:

Show changes to t/t5004-archive-corner-cases.sh +19 −0
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
Junio C Hamano· Mar 11, 2013, 05:25 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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.
Show 28 quoted lines
> 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

← back to recent threads