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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 27, 2022, 18:54 UTC
Message-ID: <20221027185437.1842331-1-jonathantanmy@google.com>
In-Reply-To: <f88ebbaf17cbf1a0b57336430bd43ade94406f38.1665973401.git.gitgitgadget@gmail.com>

"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> 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?

Show 10 quoted lines

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