# HG changeset patch # User Artem Tikhomirov # Date 1330007517 -3600 # Node ID 5e95b0da26f21921ebc8d6b753ff11079da93985 # Parent 728708de3597ff6a2e1b302cbb7a1228a6871bba Issue 24: IAE, Underflow in FilterDataAccess. Issue 26:UnsupportedOperationException when patching empty base revision. Tests diff -r 728708de3597 -r 5e95b0da26f2 src/org/tmatesoft/hg/internal/DataAccess.java --- 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) diff -r 728708de3597 -r 5e95b0da26f2 src/org/tmatesoft/hg/internal/FilterDataAccess.java --- 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); diff -r 728708de3597 -r 5e95b0da26f2 src/org/tmatesoft/hg/internal/RevlogStream.java --- 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; diff -r 728708de3597 -r 5e95b0da26f2 src/org/tmatesoft/hg/repo/HgWorkingCopyStatusCollector.java --- 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) { diff -r 728708de3597 -r 5e95b0da26f2 test-data/test-repos.jar Binary file test-data/test-repos.jar has changed diff -r 728708de3597 -r 5e95b0da26f2 test/org/tmatesoft/hg/test/TestStatus.java --- 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 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 e : sc.getErrors().entrySet()) { + System.out.printf("%s : (%s %s)\n", e.getKey(), e.getValue().getKind(), e.getValue().getMessage()); + } + assertTrue(sc.getErrors().isEmpty()); + } /*