diff mbox series

[v3,9/9] archive: add tests for git archive --recurse-submodules

Message ID f88ebbaf17cbf1a0b57336430bd43ade94406f38.1665973401.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series archive: Add --recurse-submodules to git-archive command | expand

Commit Message

Heather Lapointe Oct. 17, 2022, 2:23 a.m. UTC 
From: Heather Lapointe <alpha@alphaservcomputing.solutions>

Ensuring functionality works with and without submodules.
We expect --recurse-submodules to fail if there are uninitialized submodules
present.

Signed-off-by: Heather Lapointe <alpha@alphaservcomputing.solutions>
---
 archive.c                     |  2 +-
 t/t5005-archive-submodules.sh | 83 +++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100755 t/t5005-archive-submodules.sh

Comments

Jonathan Tan Oct. 27, 2022, 6:54 p.m. UTC | #1 
"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/archive.c b/archive.c
> index f81ef741487..b0a3181f7f5 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -179,7 +179,7 @@ static int write_archive_entry(
>  		err = write_entry(repo, args, oid, path.buf, path.len, mode, NULL, 0);
>  		if (err)
>  			return err;
> -		return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
> +		return READ_TREE_RECURSIVE;

Should this change be in the previous commit, if this commit is about 
tests? 

> +check_tar() {
> +	tarfile=$1.tar
> +	listfile=$1.lst
> +	dir=$1
> +	dir_with_prefix=$dir/$2
> +
> +	test_expect_success ' extract tar archive' '
> +		(mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile
> +	'
> +}

In the Git test codebase, there is a mix of styles in that some people 
want each test_expect_success block to be individually runnable (I am 
one of them), and to some, that's not as important. But this is 
extremely in the other direction. It would be better if each 
test_expect_success block tests one thing, but inspecting the resulting 
archive should all go into the same test_expect_success block that 
originally created the archive; we should not split each step of 
inspection into its own block. 

Also, I don't think we need to extract the tar to check it; using "tf" 
and inspecting the resulting list with "grep" and "! grep" should do.
Glen Choo Oct. 27, 2022, 11:30 p.m. UTC | #2 
Jonathan Tan <jonathantanmy@google.com> writes:

> Also, I don't think we need to extract the tar to check it; using "tf" 
> and inspecting the resulting list with "grep" and "! grep" should do. 

It might be even easier to inspect the output with test_cmp, instead of
reasoning about which files should and should not be there.
Ævar Arnfjörð Bjarmason Oct. 28, 2022, 12:17 a.m. UTC | #3 
On Mon, Oct 17 2022, Heather Lapointe via GitGitGadget wrote:

> From: Heather Lapointe <alpha@alphaservcomputing.solutions>

[In addition to what others mentioned]

> +test_description='git archive --recurse-submodules test'
> +
> +. ./test-lib.sh
> +
> +check_tar() {
> +	tarfile=$1.tar
> +	listfile=$1.lst

This "listfile" is used nowhere?"

> +	dir=$1
> +	dir_with_prefix=$dir/$2

Nor dir_with_prefix?

> +
> +	test_expect_success ' extract tar archive' '
> +		(mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile

Aside from what Jonathan mentioned, maybe we can just use one variable
here then?

	mkdir $foo ... <$foo.tar

> +	test_expect_success " validate extra file $path_in_archive" '
> +		test -f $dir/$path_in_archive &&

Instead use "test_path_is_file", and in general for "test <whatever>"
check out if we have a wrapper in test-lib-functions.sh.

> +check_not_added() {
> +	dir=$1
> +	path_in_archive=$2
> +
> +	test_expect_success " validate unpresent file $path_in_archive" '
> +		! test -f $dir/$path_in_archive &&
> +		! test -d $dir/$path_in_archive

Don't test for what a thing isn't, but what it is. Can't we do that
here?

> +test_expect_success 'setup' '
> +	rm -rf repo_with_submodules submodule1 uninited_repo_with_submodules &&

Don't have a test rm -rf stuff from a previous block, but have
"test_when_finished" clean up after that previous test instead.

> +	git init repo_with_submodules &&
> +	git init submodule1 &&
> +	(
> +		cd submodule1 &&

This:
> +		echo "dir1/sub1/file1.txt" > "file1.txt" &&
> +		git add file1.txt &&
> +		git commit -m "initialize with file1.txt"

Looks like you can use test_commit instead.

And note you can use -C, so you won't need the sub-shell either, I think.
> +	) &&
> +	(
> +	    cd repo_with_submodules &&
> +	    echo "file2" > file2.txt &&
> +	    git add file2.txt &&
> +	    git commit -m "initialize with file2.txt" &&

Ditto.

> +	    mkdir -p dir1 &&

Let's drop "-p" here, to check for errors.

> +test_expect_success 'archive with recurse, non-init' '
> +	! git -C uninited_repo_with_submodules archive --recurse-submodules -v HEAD >b2-err.tar

For git, don't use !, use test_must_fail, ! hides segfaults.

Does this test pass when you build with SANITIZE=leak? Then you can do this at the top:

	TEST_PASSES_SANITIZE_LEAK=true
	. ./test-lib.sh

If you can't test that locally pushing to GitHub CI will tell you...
diff mbox series

Patch

diff --git a/archive.c b/archive.c
index f81ef741487..b0a3181f7f5 100644
--- a/archive.c
+++ b/archive.c
@@ -179,7 +179,7 @@  static int write_archive_entry(
 		err = write_entry(repo, args, oid, path.buf, path.len, mode, NULL, 0);
 		if (err)
 			return err;
-		return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
+		return READ_TREE_RECURSIVE;
 	}
 
 	if (args->verbose)
diff --git a/t/t5005-archive-submodules.sh b/t/t5005-archive-submodules.sh
new file mode 100755
index 00000000000..aad6cfd1082
--- /dev/null
+++ b/t/t5005-archive-submodules.sh
@@ -0,0 +1,83 @@ 
+#!/bin/sh
+
+test_description='git archive --recurse-submodules test'
+
+. ./test-lib.sh
+
+check_tar() {
+	tarfile=$1.tar
+	listfile=$1.lst
+	dir=$1
+	dir_with_prefix=$dir/$2
+
+	test_expect_success ' extract tar archive' '
+		(mkdir $dir && cd $dir && "$TAR" xf -) <$tarfile
+	'
+}
+
+check_added() {
+	dir=$1
+	path_in_fs=$2
+	path_in_archive=$3
+
+	test_expect_success " validate extra file $path_in_archive" '
+		test -f $dir/$path_in_archive &&
+		diff -r $path_in_fs $dir/$path_in_archive
+	'
+}
+
+check_not_added() {
+	dir=$1
+	path_in_archive=$2
+
+	test_expect_success " validate unpresent file $path_in_archive" '
+		! test -f $dir/$path_in_archive &&
+		! test -d $dir/$path_in_archive
+	'
+}
+
+test_expect_success 'setup' '
+	rm -rf repo_with_submodules submodule1 uninited_repo_with_submodules &&
+	git init repo_with_submodules &&
+	git init submodule1 &&
+	(
+		cd submodule1 &&
+		echo "dir1/sub1/file1.txt" > "file1.txt" &&
+		git add file1.txt &&
+		git commit -m "initialize with file1.txt"
+	) &&
+	(
+	    cd repo_with_submodules &&
+	    echo "file2" > file2.txt &&
+	    git add file2.txt &&
+	    git commit -m "initialize with file2.txt" &&
+	    mkdir -p dir1 &&
+	    git submodule add ../submodule1 dir1/sub1 &&
+	    git commit -m "add submodule1"
+	) &&
+	git clone repo_with_submodules uninited_repo_with_submodules
+'
+
+test_expect_success 'archive without recurse, non-init' '
+	git -C uninited_repo_with_submodules archive -v HEAD >b.tar
+'
+
+check_tar b
+check_added b uninited_repo_with_submodules/file2.txt file2.txt
+check_not_added b uninited_repo_with_submodules/dir1/sub1/file1.txt
+
+# It is expected that --recurse-submodules will not work if submodules are not
+# initialized.
+test_expect_success 'archive with recurse, non-init' '
+	! git -C uninited_repo_with_submodules archive --recurse-submodules -v HEAD >b2-err.tar
+'
+
+test_expect_success 'archive with recurse, init' '
+	git -C repo_with_submodules archive --recurse-submodules -v HEAD >b3.tar
+'
+
+check_tar b3
+check_added b3 repo_with_submodules/file2.txt file2.txt
+check_added b3 repo_with_submodules/dir1/sub1/file1.txt dir1/sub1/file1.txt
+
+test_done