diff src/org/tmatesoft/hg/internal/RevlogStream.java @ 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 86f049e6bcae
children c76c57f6b961
line wrap: on
line diff
--- 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;