changeset 397:5e95b0da26f2 smartgit3

Issue 24: IAE, Underflow in FilterDataAccess. Issue 26:UnsupportedOperationException when patching empty base revision. Tests
author Artem Tikhomirov <tikhomirov.artem@gmail.com>
date Thu, 23 Feb 2012 15:31:57 +0100
parents 728708de3597
children c76c57f6b961 fdc1db8f7f61
files src/org/tmatesoft/hg/internal/DataAccess.java src/org/tmatesoft/hg/internal/FilterDataAccess.java src/org/tmatesoft/hg/internal/RevlogStream.java src/org/tmatesoft/hg/repo/HgWorkingCopyStatusCollector.java test-data/test-repos.jar test/org/tmatesoft/hg/test/TestStatus.java
diffstat 6 files changed, 161 insertions(+), 62 deletions(-) [+]
line wrap: on
line diff
--- a/src/org/tmatesoft/hg/internal/DataAccess.java	Tue Feb 21 19:18:40 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/DataAccess.java	Thu Feb 23 15:31:57 2012 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 TMate Software Ltd
+ * Copyright (c) 2010-2012 TMate Software Ltd
  *  
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -46,11 +46,18 @@
 	}
 	// absolute positioning
 	public void seek(int offset) throws IOException {
-		throw new UnsupportedOperationException();
+		if (offset == 0) {
+			// perfectly OK for the "empty slice" instance
+			return;
+		}
+		throw new IOException(String.format("No data, can't seek %d bytes", offset));
 	}
 	// relative positioning
 	public void skip(int bytes) throws IOException {
-		throw new UnsupportedOperationException();
+		if (bytes == 0) {
+			return;
+		}
+		throw new IOException(String.format("No data, can't skip %d bytes", bytes));
 	}
 	// shall be called once this object no longer needed
 	public void done() {
@@ -69,7 +76,10 @@
 		return ((long) i1) << 32 | ((long) i2 & 0xFFFFFFFFl);
 	}
 	public void readBytes(byte[] buf, int offset, int length) throws IOException {
-		throw new UnsupportedOperationException();
+		if (length == 0) {
+			return;
+		}
+		throw new IOException(String.format("No data, can't read %d bytes", length));
 	}
 	// reads bytes into ByteBuffer, up to its limit or total data length, whichever smaller
 	// FIXME perhaps, in DataAccess paradigm (when we read known number of bytes, we shall pass specific byte count to read) 
--- a/src/org/tmatesoft/hg/internal/FilterDataAccess.java	Tue Feb 21 19:18:40 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/FilterDataAccess.java	Thu Feb 23 15:31:57 2012 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 TMate Software Ltd
+ * Copyright (c) 2011-2012 TMate Software Ltd
  *  
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -88,7 +88,7 @@
 	@Override
 	public byte readByte() throws IOException {
 		if (count <= 0) {
-			throw new IllegalArgumentException("Underflow"); // XXX be descriptive
+			throw new IOException(String.format("Underflow. Bytes left: %d. FilterDA[offset: %d, length: %d]", count, offset, length));
 		}
 		if (count == length) {
 			dataAccess.seek(offset);
@@ -103,7 +103,7 @@
 			return;
 		}
 		if (count <= 0 || len > count) {
-			throw new IllegalArgumentException(String.format("Underflow. Bytes left: %d, asked to read %d", count, len));
+			throw new IOException(String.format("Underflow. Bytes left: %d, asked to read %d. FilterDA[offset: %d, length: %d]", count, len, offset, length));
 		}
 		if (count == length) {
 			dataAccess.seek(offset);
--- a/src/org/tmatesoft/hg/internal/RevlogStream.java	Tue Feb 21 19:18:40 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/RevlogStream.java	Thu Feb 23 15:31:57 2012 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010-2011 TMate Software Ltd
+ * Copyright (c) 2010-2012 TMate Software Ltd
  *  
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -464,7 +464,7 @@
 						daData.seek(streamOffset);
 					}
 					final boolean patchToPrevious = baseRevision != i; // the only way I found to tell if it's a patch
-					if (streamDataAccess.isEmpty()) {
+					if (streamDataAccess.isEmpty() || compressedLen == 0) {
 						userDataAccess = new DataAccess(); // empty
 					} else {
 						final byte firstByte = streamDataAccess.readByte();
@@ -474,26 +474,37 @@
 						} else if (firstByte == 0x75 /* 'u' */) {
 							userDataAccess = new FilterDataAccess(streamDataAccess, streamOffset+1, compressedLen-1);
 						} else {
-							// XXX Python impl in fact throws exception when there's not 'x', 'u' or '0'
-							// but I don't see reason not to return data as is 
+							// XXX Python impl in fact throws exception when there's not 'x', 'u' or '0' but I don't see reason not to return data as is
+							//
+							// although firstByte is already read from the streamDataAccess, FilterDataAccess#readByte would seek to
+							// initial offset before first attempt to read a byte
 							userDataAccess = new FilterDataAccess(streamDataAccess, streamOffset, compressedLen);
 						}
 					}
-					// XXX 
-					if (patchToPrevious && !userDataAccess.isEmpty() /* Issue 22, empty patch to an empty base revision*/) {
+					// userDataAccess is revision content, either complete revision, patch of a previous content, or an empty patch
+					if (patchToPrevious) {
 						// this is a patch
-						patch.read(userDataAccess);
-						userDataAccess.done();
-						//
-						// it shall be reset at the end of prev iteration, when it got assigned from userDataAccess
-						// however, actual userDataAccess and lastUserData may share Inflater object, which needs to be reset
-						// Alternatively, userDataAccess.done() above may be responsible to reset Inflater (if it's InflaterDataAccess)
-						lastUserData.reset();
-//						final long startMeasuring = System.currentTimeMillis(); // TIMING
-						byte[] userData = patch.apply(lastUserData, actualLen);
-//						applyTime += (System.currentTimeMillis() - startMeasuring); // TIMING
-						patch.clear(); // do not keep any reference, allow byte[] data to be gc'd
-						userDataAccess = new ByteArrayDataAccess(userData);
+						if (userDataAccess.isEmpty()) {
+							// Issue 22, empty patch to an empty base revision
+							// Issue 24, empty patch to non-empty base revision
+							// empty patch modifies nothing, use content of a previous revision (shall present - it's a patch here)
+							//
+							assert lastUserData.length() == actualLen; // with no patch, data size shall be the same
+							userDataAccess = lastUserData;
+						} else {
+							patch.read(userDataAccess);
+							userDataAccess.done();
+							//
+							// it shall be reset at the end of prev iteration, when it got assigned from userDataAccess
+							// however, actual userDataAccess and lastUserData may share Inflater object, which needs to be reset
+							// Alternatively, userDataAccess.done() above may be responsible to reset Inflater (if it's InflaterDataAccess)
+							lastUserData.reset();
+//							final long startMeasuring = System.currentTimeMillis(); // TIMING
+							byte[] userData = patch.apply(lastUserData, actualLen);
+//							applyTime += (System.currentTimeMillis() - startMeasuring); // TIMING
+							patch.clear(); // do not keep any reference, allow byte[] data to be gc'd
+							userDataAccess = new ByteArrayDataAccess(userData);
+						}
 					}
 				} else {
 					if (inline) {
@@ -513,7 +524,9 @@
 				if (userDataAccess != null) {
 					userDataAccess.reset(); // not sure this is necessary here, as lastUserData would get reset anyway before next use.
 				}
-				if (lastUserData != null) {
+				if (lastUserData != null && lastUserData != userDataAccess /* empty patch case, reuse of recent data in actual revision */) {
+					// release lastUserData only if we didn't reuse it in actual revision due to empty patch:
+					// empty patch means we have previous revision and didn't alter it with a patch, hence use lastUserData for userDataAccess above
 					lastUserData.done();
 				}
 				lastUserData = userDataAccess;
--- a/src/org/tmatesoft/hg/repo/HgWorkingCopyStatusCollector.java	Tue Feb 21 19:18:40 2012 +0100
+++ b/src/org/tmatesoft/hg/repo/HgWorkingCopyStatusCollector.java	Thu Feb 23 15:31:57 2012 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 TMate Software Ltd
+ * Copyright (c) 2011-2012 TMate Software Ltd
  *  
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -30,9 +30,9 @@
 import java.util.Set;
 import java.util.TreeSet;
 
-import org.tmatesoft.hg.core.HgBadStateException;
 import org.tmatesoft.hg.core.HgException;
 import org.tmatesoft.hg.core.HgInvalidControlFileException;
+import org.tmatesoft.hg.core.HgInvalidFileException;
 import org.tmatesoft.hg.core.Nodeid;
 import org.tmatesoft.hg.internal.ByteArrayChannel;
 import org.tmatesoft.hg.internal.Experimental;
@@ -291,22 +291,27 @@
 			} else if (!sizeEqual && r.size() >= 0) {
 				inspector.modified(fname);
 			} else {
-				// size is the same or unknown, and, perhaps, different timestamp
-				// check actual content to avoid false modified files
-				HgDataFile df = repo.getFileNode(fname);
-				if (!df.exists()) {
-					String msg = String.format("File %s known as normal in dirstate (%d, %d), doesn't exist at %s", fname, r.modificationTime(), r.size(), repo.getStoragePath(df));
-					throw new HgBadStateException(msg);
-				}
-				Nodeid rev = getDirstateParentManifest().nodeid(fname);
-				// rev might be null here if fname comes to dirstate as a result of a merge operation
-				// where one of the parents (first parent) had no fname file, but second parent had.
-				// E.g. fork revision 3, revision 4 gets .hgtags, few modifications and merge(3,12)
-				// see Issue 14 for details
-				if (rev == null || !areTheSame(f, df, rev)) {
-					inspector.modified(df.getPath());
-				} else {
-					inspector.clean(df.getPath());
+				try {
+					// size is the same or unknown, and, perhaps, different timestamp
+					// check actual content to avoid false modified files
+					HgDataFile df = repo.getFileNode(fname);
+					if (!df.exists()) {
+						String msg = String.format("File %s known as normal in dirstate (%d, %d), doesn't exist at %s", fname, r.modificationTime(), r.size(), repo.getStoragePath(df));
+						throw new HgInvalidFileException(msg, null).setFileName(fname);
+					}
+					Nodeid rev = getDirstateParentManifest().nodeid(fname);
+					// rev might be null here if fname comes to dirstate as a result of a merge operation
+					// where one of the parents (first parent) had no fname file, but second parent had.
+					// E.g. fork revision 3, revision 4 gets .hgtags, few modifications and merge(3,12)
+					// see Issue 14 for details
+					if (rev == null || !areTheSame(f, df, rev)) {
+						inspector.modified(df.getPath());
+					} else {
+						inspector.clean(df.getPath());
+					}
+				} catch (HgException ex) {
+					repo.getContext().getLog().warn(getClass(), ex, null);
+					inspector.invalid(fname, ex);
 				}
 			}
 		} else if ((r = getDirstateImpl().checkAdded(fname)) != null) {
@@ -383,14 +388,19 @@
 				// FALL THROUGH
 			}
 			if (r != null || (r = getDirstateImpl().checkMerged(fname)) != null || (r = getDirstateImpl().checkAdded(fname)) != null) {
-				// check actual content to see actual changes
-				// when added - seems to be the case of a file added once again, hence need to check if content is different
-				// either clean or modified
-				HgDataFile fileNode = repo.getFileNode(fname);
-				if (areTheSame(f, fileNode, nid1)) {
-					inspector.clean(fname);
-				} else {
-					inspector.modified(fname);
+				try {
+					// check actual content to see actual changes
+					// when added - seems to be the case of a file added once again, hence need to check if content is different
+					// either clean or modified
+					HgDataFile fileNode = repo.getFileNode(fname);
+					if (areTheSame(f, fileNode, nid1)) {
+						inspector.clean(fname);
+					} else {
+						inspector.modified(fname);
+					}
+				} catch (HgException ex) {
+					repo.getContext().getLog().warn(getClass(), ex, null);
+					inspector.invalid(fname, ex);
 				}
 				baseRevNames.remove(fname); // consumed, processed, handled.
 			} else if (getDirstateImpl().checkRemoved(fname) != null) {
@@ -409,10 +419,9 @@
 		// The question is whether original Hg treats this case (same content, different parents and hence nodeids) as 'modified' or 'clean'
 	}
 
-	private boolean areTheSame(FileInfo f, HgDataFile dataFile, Nodeid revision) {
+	private boolean areTheSame(FileInfo f, HgDataFile dataFile, Nodeid revision) throws HgException {
 		// XXX consider adding HgDataDile.compare(File/byte[]/whatever) operation to optimize comparison
 		ByteArrayChannel bac = new ByteArrayChannel();
-		boolean ioFailed = false;
 		try {
 			int fileRevisionIndex = dataFile.getRevisionIndex(revision);
 			// need content with metadata striped off - although theoretically chances are metadata may be different,
@@ -420,14 +429,11 @@
 			dataFile.content(fileRevisionIndex, bac);
 		} catch (CancelledException ex) {
 			// silently ignore - can't happen, ByteArrayChannel is not cancellable
-		} catch (HgException ex) {
-			repo.getContext().getLog().warn(getClass(), ex, null);
-			ioFailed = true;
 		}
-		return !ioFailed && areTheSame(f, bac.toArray(), dataFile.getPath());
+		return areTheSame(f, bac.toArray(), dataFile.getPath());
 	}
 	
-	private boolean areTheSame(FileInfo f, final byte[] data, Path p) {
+	private boolean areTheSame(FileInfo f, final byte[] data, Path p) throws HgException {
 		ReadableByteChannel is = null;
 		class Check implements ByteChannel {
 			final boolean debug = repo.getContext().getLog().isDebug(); 
@@ -498,7 +504,7 @@
 			repo.getContext().getLog().warn(getClass(), ex, "Unexpected cancellation");
 			return check.ultimatelyTheSame();
 		} catch (IOException ex) {
-			repo.getContext().getLog().warn(getClass(), ex, null);
+			throw new HgInvalidFileException("File comparison failed", ex).setFileName(p);
 		} finally {
 			if (is != null) {
 				try {
@@ -508,7 +514,6 @@
 				}
 			}
 		}
-		return false;
 	}
 
 	private static boolean todoCheckFlagsEqual(FileInfo f, HgManifest.Flags originalManifestFlags) {
Binary file test-data/test-repos.jar has changed
--- a/test/org/tmatesoft/hg/test/TestStatus.java	Tue Feb 21 19:18:40 2012 +0100
+++ b/test/org/tmatesoft/hg/test/TestStatus.java	Thu Feb 23 15:31:57 2012 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 TMate Software Ltd
+ * Copyright (c) 2011-2012 TMate Software Ltd
  *  
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -22,6 +22,7 @@
 import static org.tmatesoft.hg.core.HgStatus.Kind.*;
 import static org.tmatesoft.hg.repo.HgRepository.TIP;
 
+import java.io.File;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -527,6 +528,76 @@
 		// shall pass without exception
 		assertTrue(sc.getErrors().isEmpty());
 	}
+	
+	/**
+	 * Issue 24: IllegalArgumentException in FilterDataAccess
+	 * There were two related defects in RevlogStream
+	 *  a) for compressedLen == 0, a byte was read and FilterDataAccess  (of length 0, but it didn't help too much) was created - first byte happen to be 0.
+	 *     Patch was not applied (userDataAccess.isEmpty() check thanks to Issue 22)
+	 *  b) That FilterDataAccess (with 0 size represents patch more or less relevantly, but didn't represent actual revision) get successfully
+	 *     reassigned as lastUserData for the next iteration. And at the next step attempt to apply patch recorded in the next revision failed
+	 *     because baseRevisionData is 0 length FilterDataAccess
+	 *
+	 * Sample:
+	 *  status-5/file1 has 3 revisions, second is zero-length patch:
+	 *  Index	   	 Offset    Packed     Actual   Base Rev
+	 *     0:             0     8          7          0
+	 *  DATA
+	 *     1:             8     0          7          0
+	 *  NO DATA
+	 *     2:             8     14         6          0
+	 *  PATCH
+	 */
+	@Test
+	public void testZeroLengthPatchAgainstNonEmptyBaseRev() throws Exception{
+		repo = Configuration.get().find("status-5");
+		// pretend we modified file in the working copy
+		// for HgWorkingCopyStatusCollector to go and retrieve its content from repository 
+		File f1 = new File(repo.getWorkingDir(), "file1");
+		f1.setLastModified(System.currentTimeMillis());
+		//
+		HgStatusCommand cmd = new HgStatusCommand(repo);
+		cmd.all();
+		StatusCollector sc = new StatusCollector();
+		cmd.execute(sc);
+		// shall pass without exception
+		//
+		for (Map.Entry<Path,Status> e : sc.getErrors().entrySet()) {
+			System.out.printf("%s : (%s %s)\n", e.getKey(), e.getValue().getKind(), e.getValue().getMessage());
+		}
+		assertTrue(sc.getErrors().isEmpty());
+	}
+
+	/**
+	 * Issue 26: UnsupportedOperationException when patching empty base revision
+	 * 
+	 * Sample:
+	 *  status-5/file2 has 3 revisions, second is patch (complete revision content in a form of the patch) for empty base revision:
+	 *  Index    Offset      Packed     Actual   Base Rev
+	 *     0:         0      0          0          0
+	 *  NO DATA
+	 *     1:         0      20         7          0
+	 *  PATCH: 0..0, 7:garbage
+	 *     2:        20      16         7          0
+	 */
+	@Test
+	public void testPatchZeroLengthBaseRevision() throws Exception {
+		repo = Configuration.get().find("status-5");
+		// touch the file to force content retrieval
+		File f2 = new File(repo.getWorkingDir(), "file2");
+		f2.setLastModified(System.currentTimeMillis());
+		//
+		HgStatusCommand cmd = new HgStatusCommand(repo);
+		cmd.all();
+		StatusCollector sc = new StatusCollector();
+		cmd.execute(sc);
+		// shall pass without exception
+		//
+		for (Map.Entry<Path,Status> e : sc.getErrors().entrySet()) {
+			System.out.printf("%s : (%s %s)\n", e.getKey(), e.getValue().getKind(), e.getValue().getMessage());
+		}
+		assertTrue(sc.getErrors().isEmpty());
+	}
 
 	
 	/*