{"thread":{"id":"33150","subject":"[PATCH 0/2] fix git-archive with empty trees","startedAt":"2013-03-11T01:31:24Z","lastAt":"2013-03-11T05:25:05Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"211014","messageId":"20130311013123.GA11692@sigill.intra.peff.net","threadId":"33150","inReplyTo":null,"subject":"[PATCH 0/2] fix git-archive with empty trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-11T01:31:24Z","receivedAt":"2013-03-11T01:31:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that \"git archive\" will barf when the root tree is empty.\nInstead, it should probably return an empty archive. I doubt many people\nreally care about this corner case in practice, but it seems like we\nshould handle it more gracefully (and it's an easy fix).\n\nIt came to my attention because we track failed git-archive invocations\nat GitHub, and a particular repo had a two commits: adding some content,\nthen reverting the original commit. You can also get there with \"commit\n--allow-empty\" on a new branch (which is what the tests do).\n\nI didn't bother even looking at empty subtrees. AFAIK, git should never\nproduce them (it omits the tree entirely if there is no content in it).\nYou would have to fake it using hash-object manually. I suspect it would\nwork just fine, as we already exercise the empty-dir code paths in the\ntests I did add.\n\n  [1/2]: test-lib: factor out $GIT_UNZIP setup\n  [2/2]: archive: handle commits with an empty tree\n\n-Peff\n"},{"id":"211015","messageId":"20130311013147.GA11778@sigill.intra.peff.net","threadId":"33150","inReplyTo":"20130311013123.GA11692@sigill.intra.peff.net","subject":"[PATCH 1/2] test-lib: factor out $GIT_UNZIP setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-11T01:31:47Z","receivedAt":"2013-03-11T01:31:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We set up the $GIT_UNZIP variable and lazy prereq in\nmultiple places (and the next patch is about to add another\none). Let's factor it out to avoid repeating ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0024-crlf-archive.sh | 6 ------\n t/t5003-archive-zip.sh  | 6 ------\n t/test-lib.sh           | 6 ++++++\n 3 files changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh\nindex 5378787..4e9fa3c 100755\n--- a/t/t0024-crlf-archive.sh\n+++ b/t/t0024-crlf-archive.sh\n@@ -3,12 +3,6 @@ test_description='respect crlf in git archive'\n test_description='respect crlf in git archive'\n \n . ./test-lib.sh\n-GIT_UNZIP=${GIT_UNZIP:-unzip}\n-\n-test_lazy_prereq UNZIP '\n-\t\"$GIT_UNZIP\" -v\n-\ttest $? -ne 127\n-'\n \n test_expect_success setup '\n \ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 7cfe9ca..0f5b42b 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -3,15 +3,9 @@ SUBSTFORMAT=%H%n\n test_description='git archive --format=zip test'\n \n . ./test-lib.sh\n-GIT_UNZIP=${GIT_UNZIP:-unzip}\n \n SUBSTFORMAT=%H%n\n \n-test_lazy_prereq UNZIP '\n-\t\"$GIT_UNZIP\" -v\n-\ttest $? -ne 127\n-'\n-\n test_lazy_prereq UNZIP_SYMLINKS '\n \t(\n \t\tmkdir unzip-symlinks &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9e7f6b4..1f51025 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -760,3 +760,9 @@ test -w / || test_set_prereq SANITY\n # When the tests are run as root, permission tests will report that\n # things are writable when they shouldn't be.\n test -w / || test_set_prereq SANITY\n+\n+GIT_UNZIP=${GIT_UNZIP:-unzip}\n+test_lazy_prereq UNZIP '\n+\t\"$GIT_UNZIP\" -v\n+\ttest $? -ne 127\n+'\n-- \n1.8.2.rc3.4.gc6ed371\n"},{"id":"211016","messageId":"20130311013231.GB11778@sigill.intra.peff.net","threadId":"33150","inReplyTo":"20130311013123.GA11692@sigill.intra.peff.net","subject":"[PATCH 2/2] archive: handle commits with an empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-11T01:32:32Z","receivedAt":"2013-03-11T01:32:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"git-archive relies on get_pathspec to convert its argv into\na list of pathspecs. When get_pathspec is given an empty\nargv list, it returns a single pathspec, the empty string,\nto indicate that everything matches. When we feed this to\nour path_exists function, we typically see that the pathspec\nturns up at least one item in the tree, and we are happy.\n\nBut when our tree is empty, we erroneously think it is\nbecause the pathspec is too limited, when in fact it is\nsimply that there is nothing to be found in the tree. This\nis a weird corner case, but the correct behavior is almost\ncertainly to produce an empty archive, not to exit with an\nerror.\n\nThis patch teaches git-archive to create empty archives when\nthere is no pathspec given (we continue to complain if a\npathspec is given, since it obviously is not matched). It\nalso confirms that the tar and zip writers produce sane\noutput in this instance.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n archive.c                       |   2 +-\n t/t5004-archive-corner-cases.sh |  83 ++++++++++++++++++++++++++++++++++++++++\n t/t5004/empty.zip               | Bin 0 -> 62 bytes\n 3 files changed, 84 insertions(+), 1 deletion(-)\n create mode 100755 t/t5004-archive-corner-cases.sh\n create mode 100644 t/t5004/empty.zip\n\ndiff --git a/archive.c b/archive.c\nindex 93e00bb..d254fa5 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -234,7 +234,7 @@ static void parse_pathspec_arg(const char **pathspec,\n \tar_args->pathspec = pathspec = get_pathspec(\"\", pathspec);\n \tif (pathspec) {\n \t\twhile (*pathspec) {\n-\t\t\tif (!path_exists(ar_args->tree, *pathspec))\n+\t\t\tif (**pathspec && !path_exists(ar_args->tree, *pathspec))\n \t\t\t\tdie(\"path not found: %s\", *pathspec);\n \t\t\tpathspec++;\n \t\t}\ndiff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\nnew file mode 100755\nindex 0000000..395dd58\n--- /dev/null\n+++ b/t/t5004-archive-corner-cases.sh\n@@ -0,0 +1,83 @@\n+#!/bin/sh\n+\n+test_description='test corner cases of git-archive'\n+. ./test-lib.sh\n+\n+test_expect_success 'create commit with empty tree' '\n+\tgit commit --allow-empty -m foo\n+'\n+\n+# Make a dir and clean it up afterwards\n+make_dir() {\n+\tmkdir \"$1\" &&\n+\ttest_when_finished \"rm -rf '$1'\"\n+}\n+\n+# Check that the dir given in \"$1\" contains exactly the\n+# set of paths given as arguments.\n+check_dir() {\n+\tdir=$1; shift\n+\t{\n+\t\techo \"$dir\" &&\n+\t\tfor i in \"$@\"; do\n+\t\t\techo \"$dir/$i\"\n+\t\tdone\n+\t} | sort >expect &&\n+\tfind \"$dir\" -print | sort >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'tar archive of empty tree is empty' '\n+\tgit archive --format=tar HEAD >empty.tar &&\n+\tmake_dir extract &&\n+\t\"$TAR\" xf empty.tar -C extract &&\n+\tcheck_dir extract\n+'\n+\n+test_expect_success 'tar archive of empty tree with prefix' '\n+\tgit archive --format=tar --prefix=foo/ HEAD >prefix.tar &&\n+\tmake_dir extract &&\n+\t\"$TAR\" xf prefix.tar -C extract &&\n+\tcheck_dir extract foo\n+'\n+\n+test_expect_success UNZIP 'zip archive of empty tree is empty' '\n+\t# Detect the exit code produced when our particular flavor of unzip\n+\t# sees an empty archive. Infozip will generate a warning and exit with\n+\t# code 1. But in the name of sanity, we do not expect other unzip\n+\t# implementations to do the same thing (it would be perfectly\n+\t# reasonable to exit 0, for example).\n+\t#\n+\t# This makes our test less rigorous on some platforms (unzip may not\n+\t# handle the empty repo at all, making our later check of its exit code\n+\t# a no-op). But we cannot do anything reasonable except skip the test\n+\t# on such platforms anyway, and this is the moral equivalent.\n+\t\"$GIT_UNZIP\" \"$TEST_DIRECTORY\"/t5004/empty.zip\n+\texpect_code=$?\n+\n+\tgit archive --format=zip HEAD >empty.zip &&\n+\tmake_dir extract &&\n+\t(\n+\t\tcd extract &&\n+\t\ttest_expect_code $expect_code \"$GIT_UNZIP\" ../empty.zip\n+\t) &&\n+\tcheck_dir extract\n+'\n+\n+test_expect_success UNZIP 'zip archive of empty tree with prefix' '\n+\t# We do not have to play exit-code tricks here, because our\n+\t# result should not be empty; it has a directory in it.\n+\tgit archive --format=zip --prefix=foo/ HEAD >prefix.zip &&\n+\tmake_dir extract &&\n+\t(\n+\t\tcd extract &&\n+\t\t\"$GIT_UNZIP\" ../prefix.zip\n+\t) &&\n+\tcheck_dir extract foo\n+'\n+\n+test_expect_success 'archive complains about pathspec on empty tree' '\n+\ttest_must_fail git archive --format=tar HEAD -- foo >/dev/null\n+'\n+\n+test_done\ndiff --git a/t/t5004/empty.zip b/t/t5004/empty.zip\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..1a76bb600558dc94913a80076fe8dbdef13c1b15\nGIT binary patch\nliteral 62\nzcmWIWW@TeQ0~!oz$rh;wDW;amhRMmM$%ZCLW|oGQX-P%~24+b{=4q+M#^$N!DF&$k\nD8w?B~\n\nliteral 0\nHcmV?d00001\n\n-- \n1.8.2.rc3.4.gc6ed371\n"},{"id":"211021","messageId":"20130311045811.GA13510@sigill.intra.peff.net","threadId":"33150","inReplyTo":"20130311013123.GA11692@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fix git-archive with empty trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-11T04:58:11Z","receivedAt":"2013-03-11T04:58:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 10, 2013 at 09:31:24PM -0400, Jeff King wrote:\n\n> I noticed that \"git archive\" will barf when the root tree is empty.\n> [...]\n> I didn't bother even looking at empty subtrees. AFAIK, git should never\n> produce them (it omits the tree entirely if there is no content in it).\n> You would have to fake it using hash-object manually. I suspect it would\n> work just fine, as we already exercise the empty-dir code paths in the\n> tests I did add.\n\nCurious, I went ahead and tested this. It does indeed work as expected.\nThe following tests can be squashed into patch 2/2 if we want:\n\ndiff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\nindex 395dd58..cdb7d7a 100755\n--- a/t/t5004-archive-corner-cases.sh\n+++ b/t/t5004-archive-corner-cases.sh\n@@ -80,4 +80,23 @@ test_expect_success 'archive complains about pathspec on empty tree' '\n \ttest_must_fail git archive --format=tar HEAD -- foo >/dev/null\n '\n \n+test_expect_success 'create a commit with an empty subtree' '\n+\tempty_tree=$(git hash-object -t tree /dev/null) &&\n+\troot_tree=$(printf \"040000 tree $empty_tree\\tsub\\n\" | git mktree)\n+'\n+\n+test_expect_success 'archive empty subtree with no pathspec' '\n+\tgit archive --format=tar $root_tree >subtree-all.tar &&\n+\tmake_dir extract &&\n+\t\"$TAR\" xf subtree-all.tar -C extract &&\n+\tcheck_dir extract sub\n+'\n+\n+test_expect_success 'archive empty subtree by direct pathspec' '\n+\tgit archive --format=tar $root_tree -- sub >subtree-path.tar &&\n+\tmake_dir extract &&\n+\t\"$TAR\" xf subtree-path.tar -C extract &&\n+\tcheck_dir extract sub\n+'\n+\n test_done\n"},{"id":"211024","messageId":"7vfw02bj9q.fsf@alter.siamese.dyndns.org","threadId":"33150","inReplyTo":"20130311045811.GA13510@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fix git-archive with empty trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-11T05:25:05Z","receivedAt":"2013-03-11T05:25:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Mar 10, 2013 at 09:31:24PM -0400, Jeff King wrote:\n>\n>> I noticed that \"git archive\" will barf when the root tree is empty.\n>> [...]\n>> I didn't bother even looking at empty subtrees. AFAIK, git should never\n>> produce them (it omits the tree entirely if there is no content in it).\n>> You would have to fake it using hash-object manually. I suspect it would\n>> work just fine, as we already exercise the empty-dir code paths in the\n>> tests I did add.\n>\n> Curious, I went ahead and tested this. It does indeed work as expected.\n\nInteresting.\n\n> The following tests can be squashed into patch 2/2 if we want:\n\nWhy not---will do.  Thanks.\n\n> diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh\n> index 395dd58..cdb7d7a 100755\n> --- a/t/t5004-archive-corner-cases.sh\n> +++ b/t/t5004-archive-corner-cases.sh\n> @@ -80,4 +80,23 @@ test_expect_success 'archive complains about pathspec on empty tree' '\n>  \ttest_must_fail git archive --format=tar HEAD -- foo >/dev/null\n>  '\n>  \n> +test_expect_success 'create a commit with an empty subtree' '\n> +\tempty_tree=$(git hash-object -t tree /dev/null) &&\n> +\troot_tree=$(printf \"040000 tree $empty_tree\\tsub\\n\" | git mktree)\n> +'\n> +\n> +test_expect_success 'archive empty subtree with no pathspec' '\n> +\tgit archive --format=tar $root_tree >subtree-all.tar &&\n> +\tmake_dir extract &&\n> +\t\"$TAR\" xf subtree-all.tar -C extract &&\n> +\tcheck_dir extract sub\n> +'\n> +\n> +test_expect_success 'archive empty subtree by direct pathspec' '\n> +\tgit archive --format=tar $root_tree -- sub >subtree-path.tar &&\n> +\tmake_dir extract &&\n> +\t\"$TAR\" xf subtree-path.tar -C extract &&\n> +\tcheck_dir extract sub\n> +'\n> +\n>  test_done\n"}]}