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

[Jonathan Tan <jonathantanmy@google.com>]

"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.