Re: [PATCH JGit 5/5] added tests for the file based info cache update and made pass

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 8, 2009, 17:12 UTC
Message-ID: <20091008171245.GH9261@spearce.org>
In-Reply-To: <1253062116-13830-6-git-send-email-mr.gaffo@gmail.com>

mr.gaffo@gmail.com wrote:

> From: mike.gaffney <mike.gaffney@asolutions.com>
> Subject: Re: [PATCH JGit 5/5] added tests for the file based info cache
>	update and made pass

"and made pass" is the sneaky way of saying "and I actually
implemented what I should have implemented in the prior commit,
but didn't because ..." ?

Show 16 quoted lines

> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> index bea0b70..10ce9e3 100644
> --- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> +++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/CachedPacksInfoFileContentsGeneratorTest.java
> @@ -63,10 +63,10 @@ public void testGettingPacksContentsMultiplePacks() throws Exception {
>  		packs.add(new PackFile(TEST_IDX, TEST_PACK));
>  		
>  		StringBuilder expected = new StringBuilder();
> -		expected.append("P ").append(TEST_PACK.getName()).append("\n");
> -		expected.append("P ").append(TEST_PACK.getName()).append("\n");
> -		expected.append("P ").append(TEST_PACK.getName()).append("\n");
> -		expected.append("\n");
> +		expected.append("P ").append(TEST_PACK.getName()).append('\n');
> +		expected.append("P ").append(TEST_PACK.getName()).append('\n');
> +		expected.append("P ").append(TEST_PACK.getName()).append('\n');
> +		expected.append('\n');

This should be squashed to the patch that introduced the code,
not be twiddled in something that is completely unrelated to it.

Show 17 quoted lines

> diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/InfoDirectoryDatabaseTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/InfoDirectoryDatabaseTest.java
> +	public void testUpdateInfoCache() throws Exception {
> +		Collection<Ref> refs = new ArrayList<Ref>();
> +		refs.add(new Ref(Ref.Storage.LOOSE, "refs/heads/master", ObjectId.fromString("32aae7aef7a412d62192f710f2130302997ec883")));
> +		refs.add(new Ref(Ref.Storage.LOOSE, "refs/heads/development", ObjectId.fromString("184063c9b594f8968d61a686b2f6052779551613")));
> +
> +		File expectedFile = new File(testDir, "refs");
> +		assertFalse(expectedFile.exists());
> +		
> +		
> +		final StringWriter expectedString = new StringWriter();
> +		new RefWriter(refs) {
> +			@Override
> +			protected void writeFile(String file, byte[] content) throws IOException {
> +				expectedString.write(new String(content));
> +			}
> +		}.writeInfoRefs();

This feels a bit too much like testing the formatting code by relying on the formatting code to produce the correct output.

Its a 2 line file with a very well known format that cannot change without breaking every Git HTTP client out in the wild. We will not break those clients anytime in the next few years. You have the data hardcoded above *anyway*, hardcode the expected result here to ensure we formatted it right.

Oh, and IIRC order doesn't matter in the file but I think almost everyone assumes the order is as per git ls-remote, which matches the order produced by RefComparator. Which means you want to assert that development comes before master, and that tags come before heads.

Also, we need to assert that the peeled information for a tag appears in the file. So you need a tag ref with a peeled ObjectId available.

Show 13 quoted lines

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDirectoryDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/InfoDirectoryDatabase.java
> @@ -51,4 +54,16 @@ public void create() {
>  		info.mkdirs();
>  	}
>  
> +	@Override
> +	public void updateInfoCache(Collection<Ref> refs) throws IOException {
> +		new RefWriter(refs) {
> +			@Override
> +			protected void writeFile(String file, byte[] content) throws IOException {
> +				FileOutputStream fos = new FileOutputStream(new File(info, "refs"));
> +				fos.write(content);
> +				fos.close();

I think you need to use a LockFile to avoid races between readers and writers.

-- 
Shawn.