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

["Shawn O. Pearce" <spearce@spearce.org>]

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

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