Mercurial > hg4j

changeset 398:c76c57f6b961

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Merge fixed for issue 24 and issue 26 from smartgit3 branch
author Artem Tikhomirov <tikhomirov.artem@gmail.com>
date Thu, 23 Feb 2012 21:53:21 +0100
parents 0ae53c32ecef (current diff) 5e95b0da26f2 (diff)
children deb64baaa412
files src/org/tmatesoft/hg/internal/RevlogStream.java
diffstat 6 files changed, 160 insertions(+), 61 deletions(-) [+]
line wrap: on
line diff
--- a/src/org/tmatesoft/hg/internal/DataAccess.java	Thu Feb 23 01:06:24 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/DataAccess.java	Thu Feb 23 21:53:21 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	Thu Feb 23 01:06:24 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/FilterDataAccess.java	Thu Feb 23 21:53:21 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	Thu Feb 23 01:06:24 2012 +0100
+++ b/src/org/tmatesoft/hg/internal/RevlogStream.java	Thu Feb 23 21:53:21 2012 +0100
@@ -488,7 +488,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();
@@ -498,26 +498,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) {
@@ -537,7 +548,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	Thu Feb 23 01:06:24 2012 +0100
+++ b/src/org/tmatesoft/hg/repo/HgWorkingCopyStatusCollector.java	Thu Feb 23 21:53:21 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	Thu Feb 23 01:06:24 2012 +0100
+++ b/test/org/tmatesoft/hg/test/TestStatus.java	Thu Feb 23 21:53:21 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());
+	}
 
 	
 	/*