changeset 624:507602cb4fb3

FIXMEs and TODOs: pay some technical debt
author Artem Tikhomirov <tikhomirov.artem@gmail.com>
date Mon, 20 May 2013 20:34:33 +0200 (2013-05-20)
parents fedc54356091
children b4948b159ab1
files src/org/tmatesoft/hg/internal/DiffHelper.java src/org/tmatesoft/hg/internal/FNCacheFile.java src/org/tmatesoft/hg/internal/FileHistory.java src/org/tmatesoft/hg/internal/FileUtils.java src/org/tmatesoft/hg/internal/LineReader.java src/org/tmatesoft/hg/repo/HgBookmarks.java src/org/tmatesoft/hg/repo/ext/MqManager.java test/org/tmatesoft/hg/test/TestBlame.java test/org/tmatesoft/hg/test/TestCommit.java test/org/tmatesoft/hg/test/TestDiffHelper.java
diffstat 10 files changed, 90 insertions(+), 37 deletions(-) [+]
line wrap: on
line diff
--- a/src/org/tmatesoft/hg/internal/DiffHelper.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/internal/DiffHelper.java	Mon May 20 20:34:33 2013 +0200
@@ -20,8 +20,6 @@
 import java.util.HashMap;
 import java.util.Map;
 
-import org.tmatesoft.hg.repo.HgInvalidStateException;
-
 /**
  * Mercurial cares about changes only up to the line level, e.g. a simple file version dump in manifest looks like (RevlogDump output):
  * 
@@ -201,9 +199,7 @@
 				} else {
 					assert changeStartS2 == matchStartSeq2;
 					if (matchStartSeq1 > 0 || matchStartSeq2 > 0) {
-						// FIXME perhaps, exception is too much for the case
-						// once diff is covered with tests, replace with assert false : msg; 
-						throw new HgInvalidStateException(String.format("adjustent equal blocks %d, %d and %d,%d", changeStartS1, matchStartSeq1, changeStartS2, matchStartSeq2));
+						assert false : String.format("adjustent equal blocks %d, %d and %d,%d", changeStartS1, matchStartSeq1, changeStartS2, matchStartSeq2);
 					}
 				}
 			}
--- a/src/org/tmatesoft/hg/internal/FNCacheFile.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/internal/FNCacheFile.java	Mon May 20 20:34:33 2013 +0200
@@ -68,7 +68,7 @@
 		// names in fncache are in local encoding, shall translate to unicode
 		new LineReader(f, repo.getSessionContext().getLog(), repo.getFilenameEncoding()).read(new LineReader.SimpleLineCollector(), entries);
 		for (String e : entries) {
-			// FIXME plain wrong, need either to decode paths and strip off .i/.d or (if keep names as is) change write()
+			// XXX plain wrong, need either to decode paths and strip off .i/.d or (if keep names as is) change write()
 			files.add(pathFactory.path(e));
 		}
 	}
--- a/src/org/tmatesoft/hg/internal/FileHistory.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/internal/FileHistory.java	Mon May 20 20:34:33 2013 +0200
@@ -30,7 +30,7 @@
  * History of a file, with copy/renames, and corresponding revision information.
  * Facility for file history iteration. 
  * 
- * FIXME [post-1.1] Utilize in HgLogCommand and anywhere else we need to follow file history
+ * TODO [post-1.1] Utilize in HgLogCommand and anywhere else we need to follow file history
  * 
  * @author Artem Tikhomirov
  * @author TMate Software Ltd.
--- a/src/org/tmatesoft/hg/internal/FileUtils.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/internal/FileUtils.java	Mon May 20 20:34:33 2013 +0200
@@ -82,6 +82,10 @@
 			String m = String.format("Failed to copy %s to %s", from.getName(), to.getName());
 			throw new HgIOException(m, ex, from);
 		}
+		/* Copy of cpython's 00changelog.d, 20Mb+
+		 * Linux&Windows: 300-400 ms,
+		 * Windows uncached run: 1.6 seconds
+		 */
 	}
 	
 	public void closeQuietly(Closeable stream) {
@@ -94,12 +98,4 @@
 			}
 		}
 	}
-
-	public static void main(String[] args) throws Exception {
-		final long start = System.nanoTime();
-		final File src = new File(".../hg/cpython/.hg/store/00changelog.d");
-		copyFile(src, new File("/tmp/zxczxczxc234"));
-		final long end = System.nanoTime();
-		System.out.printf("Copy of %,d bytes took %d ms", src.length(), (end-start)/1000000);
-	}
 }
--- a/src/org/tmatesoft/hg/internal/LineReader.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/internal/LineReader.java	Mon May 20 20:34:33 2013 +0200
@@ -16,8 +16,6 @@
  */
 package org.tmatesoft.hg.internal;
 
-import static org.tmatesoft.hg.util.LogFacility.Severity.Warn;
-
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
@@ -29,7 +27,6 @@
 import java.util.Collection;
 
 import org.tmatesoft.hg.repo.HgInvalidFileException;
-import org.tmatesoft.hg.repo.ext.MqManager;
 import org.tmatesoft.hg.util.LogFacility;
 
 /**
@@ -125,17 +122,11 @@
 			} catch (IOException ex) {
 				throw new HgInvalidFileException(ex.getMessage(), ex, file);
 			} finally {
-				if (statusFileReader != null) {
-					try {
-						statusFileReader.close();
-					} catch (IOException ex) {
-						log.dump(MqManager.class, Warn, ex, null);
-					}
-				}
+				new FileUtils(log).closeQuietly(statusFileReader);
 //				try {
 //					consumer.end(file, paramObj);
 //				} catch (IOException ex) {
-//					log.warn(MqManager.class, ex, null);
+//					log.warn(getClass(), ex, null);
 //				}
 			}
 		}
--- a/src/org/tmatesoft/hg/repo/HgBookmarks.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/repo/HgBookmarks.java	Mon May 20 20:34:33 2013 +0200
@@ -167,10 +167,7 @@
 		}
 		Nodeid activeRev = getRevision(activeBookmark);
 		if (!activeRev.equals(p1) && !activeRev.equals(p2)) {
-			// from the wiki:
-			// "active bookmarks are automatically updated when committing to the changeset they are pointing to"
-			// FIXME: test ci 1, hg bookmark active, ci 2, hg bookmark -f -r 0 active, ci 3, check active still points to r0 
-			return;
+			return; // TestCommit#testNoBookmarkUpdate
 		}
 		if (child.equals(activeRev)) {
 			return;
--- a/src/org/tmatesoft/hg/repo/ext/MqManager.java	Mon May 20 18:35:13 2013 +0200
+++ b/src/org/tmatesoft/hg/repo/ext/MqManager.java	Mon May 20 20:34:33 2013 +0200
@@ -39,9 +39,6 @@
  * Mercurial Queues Support. 
  * Access to MqExtension functionality.
  * 
- * FIXME check we don't hold any mq files for too long, close them, use
- * the same lock mechanism as mq does (if any). Check if MQ uses Mercurial's store lock
- * 
  * @since 1.1
  * @author Artem Tikhomirov
  * @author TMate Software Ltd.
@@ -65,6 +62,8 @@
 	 * @return <code>this</code> for convenience
 	 */
 	public MqManager refresh() throws HgInvalidControlFileException {
+		// MQ doesn't seem to use any custom lock mechanism.
+		// MQ uses Mercurial's wc/store lock when updating repository (strip/new queue)
 		applied = allKnown = Collections.emptyList();
 		queueNames = Collections.emptyList();
 		final LogFacility log = repo.getSessionContext().getLog();
--- a/test/org/tmatesoft/hg/test/TestBlame.java	Mon May 20 18:35:13 2013 +0200
+++ b/test/org/tmatesoft/hg/test/TestBlame.java	Mon May 20 20:34:33 2013 +0200
@@ -255,7 +255,7 @@
 		}
 	}
 
-	// FIXME HgWorkingCopyStatusCollector (and HgStatusCollector), with their ancestors (rev 59/69) have examples
+	// TODO HgWorkingCopyStatusCollector (and HgStatusCollector), with their ancestors (rev 59/69) have examples
 	// of *incorrect* assignment of common lines (like "}") - our impl doesn't process common lines in any special way
 	// while original diff lib does. Would be nice to behave as close to original, as possible.
 	
--- a/test/org/tmatesoft/hg/test/TestCommit.java	Mon May 20 18:35:13 2013 +0200
+++ b/test/org/tmatesoft/hg/test/TestCommit.java	Mon May 20 20:34:33 2013 +0200
@@ -75,8 +75,6 @@
 		//
 		HgRepository hgRepo = new HgLookup().detect(repoLoc);
 		CommitFacility cf = new CommitFacility(Internals.getInstance(hgRepo), 0);
-		// FIXME test diff for processing changed newlines (ie \r\n -> \n or vice verse) - if a whole line or 
-		// just changed endings are in the patch!
 		HgDataFile df = hgRepo.getFileNode("file1");
 		cf.add(df, new ByteArrayDataSource("hello\nworld".getBytes()));
 		Transaction tr = newTransaction(hgRepo);
@@ -319,6 +317,42 @@
 		errorCollector.assertEquals(c, hgRepo.getBookmarks().getRevision(activeBookmark));
 	}
 
+	/**
+	 * from the wiki:
+	 * "active bookmarks are automatically updated when committing to the changeset they are pointing to"
+	 * Synopsis: commit 1 (c1), hg bookmark active (points to commit1), make commit 2, hg bookmark -f -r c1 active, commit 3, check active still points to c1 
+	 */
+	@Test
+	public void testNoBookmarkUpdate() throws Exception {
+		File repoLoc = RepoUtils.cloneRepoToTempLocation("log-1", "test-no-bookmark-upd", false);
+		HgRepository hgRepo = new HgLookup().detect(repoLoc);
+		assertNull("[sanity]", hgRepo.getBookmarks().getActiveBookmarkName());
+		ExecHelper eh = new ExecHelper(new OutputParser.Stub(), repoLoc);
+		String activeBookmark = "bm1";
+		eh.run("hg", "bookmarks", activeBookmark);
+		assertEquals("Bookmarks has to reload", activeBookmark, hgRepo.getBookmarks().getActiveBookmarkName());
+		Nodeid initialBookmarkRevision = hgRepo.getBookmarks().getRevision(activeBookmark); // c1
+		assertEquals("[sanity]", initialBookmarkRevision, hgRepo.getWorkingCopyParents().first());
+
+		File fileD = new File(repoLoc, "d");
+		assertTrue("[sanity]", fileD.canRead());
+		RepoUtils.modifyFileAppend(fileD, " 1 \n");
+		HgCommitCommand cmd = new HgCommitCommand(hgRepo).message("FIRST");
+		Outcome r = cmd.execute();
+		errorCollector.assertTrue(r.isOk());
+		Nodeid c2 = cmd.getCommittedRevision();
+		errorCollector.assertEquals(c2, hgRepo.getBookmarks().getRevision(activeBookmark));
+		//
+		eh.run("hg", "bookmark", activeBookmark, "--force", "--rev", initialBookmarkRevision.toString());
+		//
+		RepoUtils.modifyFileAppend(fileD, " 2 \n");
+		cmd = new HgCommitCommand(hgRepo).message("SECOND");
+		r = cmd.execute();
+		errorCollector.assertTrue(r.isOk());
+		//Nodeid c3 = cmd.getCommittedRevision();
+		errorCollector.assertEquals(initialBookmarkRevision, hgRepo.getBookmarks().getRevision(activeBookmark));
+	}
+
 	@Test
 	public void testRefreshTagsAndBranches() throws Exception {
 		File repoLoc = RepoUtils.cloneRepoToTempLocation("log-branches", "test-refresh-after-commit", false);
--- a/test/org/tmatesoft/hg/test/TestDiffHelper.java	Mon May 20 18:35:13 2013 +0200
+++ b/test/org/tmatesoft/hg/test/TestDiffHelper.java	Mon May 20 20:34:33 2013 +0200
@@ -118,6 +118,42 @@
 		diff.findMatchingBlocks(mc = new MatchCollector<CharSequence>());
 		assertEquals(3, mc.matchCount()); // bc, e, g
 	}
+
+	@Test
+	public void testChangedEOL() {
+		DiffHelper<LineSequence> diffHelper = new DiffHelper<LineSequence>();
+		MatchCollector<LineSequence> mc; DeltaCollector dc;
+		// all lines changed
+		diffHelper.init(newlines("one\ntwo\nthree\n".getBytes()), newlines("one\r\ntwo\r\nthree\r\n".getBytes()));
+		diffHelper.findMatchingBlocks(mc = new MatchCollector<LineSequence>());
+		assertEquals(0, mc.matchCount());
+		diffHelper.findMatchingBlocks(dc = new DeltaCollector());
+		assertEquals(0, dc.unchangedCount());
+		assertEquals(1, dc.deletedCount());
+		assertTrue(dc.deletedLine(0));
+		assertTrue(dc.deletedLine(1));
+		assertTrue(dc.deletedLine(2));
+		assertEquals(1, dc.addedCount());
+		assertTrue(dc.addedLine(0));
+		assertTrue(dc.addedLine(1));
+		assertTrue(dc.addedLine(2));
+		// one line changed
+		diffHelper.init(newlines("one\ntwo\nthree\n".getBytes()), newlines("one\ntwo\r\nthree\n".getBytes()));
+		diffHelper.findMatchingBlocks(mc = new MatchCollector<LineSequence>());
+		assertEquals(2, mc.matchCount());
+		assertTrue(mc.originLineMatched(0));
+		assertTrue(mc.targetLineMatched(0));
+		assertFalse(mc.originLineMatched(1));
+		assertFalse(mc.targetLineMatched(1));
+		assertTrue(mc.originLineMatched(2));
+		assertTrue(mc.targetLineMatched(2));
+		diffHelper.findMatchingBlocks(dc = new DeltaCollector());
+		assertEquals(2, dc.unchangedCount());
+		assertEquals(1, dc.deletedCount());
+		assertTrue(dc.deletedLine(1));
+		assertEquals(1, dc.addedCount());
+		assertTrue(dc.addedLine(1));
+	}
 	
 	// range is comprised of 3 values, range length always last, range start comes at index o (either 0 or 1)
 	static boolean includes(IntVector ranges, int o, int ln) {
@@ -188,20 +224,24 @@
 			same.add(s1From, s2From, length);
 		}
 
+		// return number of regions that didn't change
 		int unchangedCount() {
 			return same.size() / 3;
 		}
 
+		// return number of added regions
 		int addedCount() {
 			return added.size() / 3;
 		}
-
+		// return number of deleted regions
 		int deletedCount() {
 			return deleted.size() / 3;
 		}
+		// answer if 0-based line is marked as added
 		boolean addedLine(int ln) {
 			return includes(added, 1, ln);
 		}
+		// answer if 0-based line is marked as deleted
 		boolean deletedLine(int ln) {
 			return includes(deleted, 1, ln);
 		}