]> source.dussan.org Git - jgit.git/commitdiff
Clarify WorkingTreeOptions and filemode usage 67/2067/2
authorShawn O. Pearce <spearce@spearce.org>
Wed, 8 Dec 2010 02:13:36 +0000 (18:13 -0800)
committerShawn O. Pearce <spearce@spearce.org>
Wed, 8 Dec 2010 18:03:19 +0000 (10:03 -0800)
To improve runtime performance, caching the WorkingTreeOptions inside
of the Config object using the Config.SectionParser API allows
the WorkingTreeOptions to be accessed more efficiently whenever a
FileTreeIterator is constructed for the Repository.

Instead of passing the filemode handling option into isModified(),
the WorkingTreeIterator should always honor whatever setting has
been configured in this repository, as defined by its own copy of
the WorkingTreeOptions.  This simplifies all of the callers as they
no longer need to lookup core.filemode on their own.

A few locations were changed from always using a hardcoded "true"
on the file mode to passing what is actually configured in the
repository.  This is a behavior change, but corrects what should be
considered to be bugs as the core.filemode variable wasn't always
being used.

Change-Id: Idb176736fa0dc97af372f1d652a94ecc72fb457c
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/opt/AbstractTreeIteratorHandler.java
org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/AbstractTreeIteratorTest.java
org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java
org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorWithTimeControl.java
org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java
org.eclipse.jgit/src/org/eclipse/jgit/lib/CoreConfig.java
org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeOptions.java

index 8d0b504c3770ada4346d988ae06fa80c4539da25..ca9710491c016553e766229f5c877dfee8a5109b 100644 (file)
@@ -97,7 +97,10 @@ public class AbstractTreeIteratorHandler extends
                final String name = params.getParameter(0);
 
                if (new File(name).isDirectory()) {
-                       setter.addValue(new FileTreeIterator(new File(name), FS.DETECTED, WorkingTreeOptions.createDefaultInstance()));
+                       setter.addValue(new FileTreeIterator(
+                               new File(name),
+                               FS.DETECTED,
+                               clp.getRepository().getConfig().get(WorkingTreeOptions.KEY)));
                        return 1;
                }
 
index cc7d0ad9ef10e22a6bd1d1845dab8676f2e2eddc..86b5a0f62a01fa7b1cd51e20691bb8abff795eb4 100644 (file)
@@ -49,10 +49,10 @@ import java.io.IOException;
 import junit.framework.TestCase;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectReader;
-import org.eclipse.jgit.lib.CoreConfig.AutoCRLF;
 
 
 public class AbstractTreeIteratorTest extends TestCase {
@@ -63,7 +63,7 @@ public class AbstractTreeIteratorTest extends TestCase {
 
        public class FakeTreeIterator extends WorkingTreeIterator {
                public FakeTreeIterator(String pathName, FileMode fileMode) {
-                       super(prefix(pathName), new WorkingTreeOptions(AutoCRLF.FALSE));
+                       super(prefix(pathName), new Config().get(WorkingTreeOptions.KEY));
                        mode = fileMode.getBits();
 
                        final int s = pathName.lastIndexOf('/');
index ae519f4eab5a190dae70e8b65e8ac1dea49bae6e..22eeb8e3d3a3a6aa6b10bf6168c5e90be475d845 100644 (file)
@@ -80,7 +80,7 @@ public class FileTreeIteratorTest extends RepositoryTestCase {
                final File r = new File(trash, paths[0]);
                assertTrue(r.isFile());
                final FileTreeIterator fti = new FileTreeIterator(r, db.getFS(),
-                               WorkingTreeOptions.createConfigurationInstance(db.getConfig()));
+                               db.getConfig().get(WorkingTreeOptions.KEY));
                assertTrue(fti.first());
                assertTrue(fti.eof());
        }
@@ -89,7 +89,7 @@ public class FileTreeIteratorTest extends RepositoryTestCase {
                final File r = new File(trash, "not-existing-file");
                assertFalse(r.exists());
                final FileTreeIterator fti = new FileTreeIterator(r, db.getFS(),
-                               WorkingTreeOptions.createConfigurationInstance(db.getConfig()));
+                               db.getConfig().get(WorkingTreeOptions.KEY));
                assertTrue(fti.first());
                assertTrue(fti.eof());
        }
@@ -101,14 +101,14 @@ public class FileTreeIteratorTest extends RepositoryTestCase {
                assertTrue(r.isDirectory());
 
                final FileTreeIterator fti = new FileTreeIterator(r, db.getFS(),
-                               WorkingTreeOptions.createConfigurationInstance(db.getConfig()));
+                               db.getConfig().get(WorkingTreeOptions.KEY));
                assertTrue(fti.first());
                assertTrue(fti.eof());
        }
 
        public void testSimpleIterate() throws Exception {
                final FileTreeIterator top = new FileTreeIterator(trash, db.getFS(),
-                               WorkingTreeOptions.createConfigurationInstance(db.getConfig()));
+                               db.getConfig().get(WorkingTreeOptions.KEY));
 
                assertTrue(top.first());
                assertFalse(top.eof());
@@ -157,7 +157,7 @@ public class FileTreeIteratorTest extends RepositoryTestCase {
 
        public void testComputeFileObjectId() throws Exception {
                final FileTreeIterator top = new FileTreeIterator(trash, db.getFS(),
-                               WorkingTreeOptions.createConfigurationInstance(db.getConfig()));
+                               db.getConfig().get(WorkingTreeOptions.KEY));
 
                final MessageDigest md = Constants.newMessageDigest();
                md.update(Constants.encodeASCII(Constants.TYPE_BLOB));
index f15454ba07cfc2b59ae5416ad998656d6e673244..8ea1fd9e98b6b36c09408e728339dcbf4ae134ed 100644 (file)
@@ -46,9 +46,9 @@ import java.io.File;
 import java.util.SortedSet;
 import java.util.TreeSet;
 
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.lib.CoreConfig.AutoCRLF;
 import org.eclipse.jgit.util.FS;
 
 /**
@@ -88,7 +88,7 @@ public class FileTreeIteratorWithTimeControl extends FileTreeIterator {
 
        public FileTreeIteratorWithTimeControl(File f, FS fs,
                        TreeSet<Long> modTimes) {
-               super(f, fs, new WorkingTreeOptions(AutoCRLF.FALSE));
+               super(f, fs, new Config().get(WorkingTreeOptions.KEY));
                this.modTimes = modTimes;
        }
 
index 1e08d98a2e2789cac48ba956f58358ab7c1532b0..c9df809afe399df98c3b503eeaac6b9c25e5cede 100644 (file)
@@ -61,7 +61,6 @@ import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.treewalk.AbstractTreeIterator;
 import org.eclipse.jgit.treewalk.CanonicalTreeParser;
 import org.eclipse.jgit.treewalk.EmptyTreeIterator;
@@ -184,9 +183,7 @@ public class DirCacheCheckout {
         */
        public DirCacheCheckout(Repository repo, ObjectId headCommitTree,
                        DirCache dc, ObjectId mergeCommitTree) throws IOException {
-               this(repo, headCommitTree, dc, mergeCommitTree, new FileTreeIterator(
-                               repo.getWorkTree(), repo.getFS(),
-                               WorkingTreeOptions.createDefaultInstance()));
+               this(repo, headCommitTree, dc, mergeCommitTree, new FileTreeIterator(repo));
        }
 
        /**
@@ -224,9 +221,7 @@ public class DirCacheCheckout {
         */
        public DirCacheCheckout(Repository repo, DirCache dc,
                        ObjectId mergeCommitTree) throws IOException {
-               this(repo, null, dc, mergeCommitTree, new FileTreeIterator(
-                               repo.getWorkTree(), repo.getFS(),
-                               WorkingTreeOptions.createDefaultInstance()));
+               this(repo, null, dc, mergeCommitTree, new FileTreeIterator(repo));
        }
 
        /**
@@ -311,7 +306,7 @@ public class DirCacheCheckout {
                if (m != null) {
                        if (i == null || f == null || !m.idEqual(i)
                                        || (i.getDirCacheEntry() != null && (f.isModified(
-                                                       i.getDirCacheEntry(), true, config_filemode()) ||
+                                                       i.getDirCacheEntry(), true) ||
                                                        i.getDirCacheEntry().getStage() != 0))) {
                                update(m.getEntryPathString(), m.getEntryObjectId(),
                                                m.getEntryFileMode());
@@ -391,7 +386,7 @@ public class DirCacheCheckout {
                        file.getParentFile().mkdirs();
                        file.createNewFile();
                        DirCacheEntry entry = dc.getEntry(path);
-                       checkoutEntry(repo, file, entry, config_filemode());
+                       checkoutEntry(repo, file, entry);
                }
 
 
@@ -575,7 +570,7 @@ public class DirCacheCheckout {
                        case 0xFFD: // 12 13 14
                                if (hId.equals(iId)) {
                                        dce = i.getDirCacheEntry();
-                                       if (f == null || f.isModified(dce, true, config_filemode()))
+                                       if (f == null || f.isModified(dce, true))
                                                conflict(name, i.getDirCacheEntry(), h, m);
                                        else
                                                remove(name);
@@ -641,8 +636,7 @@ public class DirCacheCheckout {
                                if (m == null || mId.equals(iId)) {
                                        if (m==null && walk.isDirectoryFileConflict()) {
                                                if (dce != null
-                                                               && (f == null || f.isModified(dce, true,
-                                                                               config_filemode())))
+                                                               && (f == null || f.isModified(dce, true)))
                                                        conflict(name, i.getDirCacheEntry(), h, m);
                                                else
                                                        remove(name);
@@ -664,7 +658,7 @@ public class DirCacheCheckout {
                                 */
 
                                if (hId.equals(iId)) {
-                                       if (f == null || f.isModified(dce, true, config_filemode()))
+                                       if (f == null || f.isModified(dce, true))
                                                conflict(name, i.getDirCacheEntry(), h, m);
                                        else
                                                remove(name);
@@ -674,9 +668,7 @@ public class DirCacheCheckout {
                                if (!hId.equals(mId) && !hId.equals(iId) && !mId.equals(iId))
                                        conflict(name, i.getDirCacheEntry(), h, m);
                                else if (hId.equals(iId) && !mId.equals(iId)) {
-                                       if (dce != null
-                                                       && (f == null || f.isModified(dce, true,
-                                                                       config_filemode())))
+                                       if (dce != null && (f == null || f.isModified(dce, true)))
                                                conflict(name, i.getDirCacheEntry(), h, m);
                                        else
                                                update(name, mId, m.getEntryFileMode());
@@ -738,19 +730,6 @@ public class DirCacheCheckout {
                }
        }
 
-       private Boolean filemode;
-
-       private boolean config_filemode() {
-               // TODO: temporary till we can actually set parameters. We need to be
-               // able to change this for testing.
-               if (filemode == null) {
-                       StoredConfig config = repo.getConfig();
-                       filemode = Boolean.valueOf(config.getBoolean("core", null,
-                                       "filemode", true));
-               }
-               return filemode.booleanValue();
-       }
-
        /**
         * If <code>true</code>, will scan first to see if it's possible to check
         * out, otherwise throw {@link CheckoutConflictException}. If
@@ -790,8 +769,7 @@ public class DirCacheCheckout {
        private boolean isModified(String path) throws CorruptObjectException, IOException {
                NameConflictTreeWalk tw = new NameConflictTreeWalk(repo);
                tw.addTree(new DirCacheIterator(dc));
-               tw.addTree(new FileTreeIterator(repo.getWorkTree(), repo.getFS(),
-                               WorkingTreeOptions.createDefaultInstance()));
+               tw.addTree(new FileTreeIterator(repo));
                tw.setRecursive(true);
                tw.setFilter(PathFilter.create(path));
                DirCacheIterator dcIt;
@@ -801,8 +779,7 @@ public class DirCacheCheckout {
                        wtIt = tw.getTree(1, WorkingTreeIterator.class);
                        if (dcIt == null || wtIt == null)
                                return true;
-                       if (wtIt.isModified(dcIt.getDirCacheEntry(), true,
-                                       config_filemode())) {
+                       if (wtIt.isModified(dcIt.getDirCacheEntry(), true)) {
                                return true;
                        }
                }
@@ -824,12 +801,10 @@ public class DirCacheCheckout {
         *            has to exist already
         * @param entry
         *            the entry containing new mode and content
-        * @param config_filemode
-        *            whether the mode bits should be handled at all.
         * @throws IOException
         */
-       public static void checkoutEntry(final Repository repo, File f, DirCacheEntry entry,
-                       boolean config_filemode) throws IOException {
+       public static void checkoutEntry(final Repository repo, File f,
+                       DirCacheEntry entry) throws IOException {
                ObjectLoader ol = repo.open(entry.getObjectId());
                File parentDir = f.getParentFile();
                File tmpFile = File.createTempFile("._" + f.getName(), null, parentDir);
@@ -840,7 +815,8 @@ public class DirCacheCheckout {
                        channel.close();
                }
                FS fs = repo.getFS();
-               if (config_filemode && fs.supportsExecute()) {
+               WorkingTreeOptions opt = repo.getConfig().get(WorkingTreeOptions.KEY);
+               if (opt.isFileMode() && fs.supportsExecute()) {
                        if (FileMode.EXECUTABLE_FILE.equals(entry.getRawMode())) {
                                if (!fs.canExecute(tmpFile))
                                        fs.setExecute(tmpFile, true);
index 462614c86f283bac073fb659c044c5869c14d914..712f6e4650ceafde02711aa0dc2ed701d6372945 100644 (file)
@@ -80,16 +80,10 @@ public class CoreConfig {
 
        private final boolean logAllRefUpdates;
 
-       private final boolean fileMode;
-
-       private final AutoCRLF autoCRLF;
-
        private CoreConfig(final Config rc) {
                compression = rc.getInt("core", "compression", DEFAULT_COMPRESSION);
                packIndexVersion = rc.getInt("pack", "indexversion", 2);
                logAllRefUpdates = rc.getBoolean("core", "logallrefupdates", true);
-               fileMode = rc.getBoolean("core", "filemode", true);
-               autoCRLF = rc.getEnum("core", null, "autocrlf", AutoCRLF.FALSE);
        }
 
        /**
@@ -113,18 +107,4 @@ public class CoreConfig {
        public boolean isLogAllRefUpdates() {
                return logAllRefUpdates;
        }
-
-       /**
-        * @return whether to trust file modes
-        */
-       public boolean isFileMode() {
-               return fileMode;
-       }
-
-       /**
-        * @return whether automatic CRLF conversion has been configured
-        */
-       public AutoCRLF getAutoCRLF() {
-               return autoCRLF;
-       }
 }
index dbb04deadffad29f71de407562ae1df8b36e72b3..15309fcefc6159482bb3168c4da942f99fc186a7 100644 (file)
@@ -244,7 +244,7 @@ public class ResolveMerger extends ThreeWayMerger {
                                createDir(f.getParentFile());
                                DirCacheCheckout.checkoutEntry(db,
                                                f,
-                                               entry.getValue(), true);
+                                               entry.getValue());
                        } else {
                                if (!f.delete())
                                        failingPathes.put(entry.getKey(),
@@ -446,8 +446,7 @@ public class ResolveMerger extends ThreeWayMerger {
                                // is not modified
                                if (work != null
                                                && (!nonTree(work.getEntryRawMode()) || work
-                                                               .isModified(index.getDirCacheEntry(), true,
-                                                                               true))) {
+                                                               .isModified(index.getDirCacheEntry(), true))) {
                                        failingPathes.put(tw.getPathString(),
                                                        MergeFailureReason.DIRTY_WORKTREE);
                                        return false;
index 8f53a0df90a1224ed1f495d620075382d77d414e..c683308e92874dd581c96411b6fba953bf7bea9b 100644 (file)
@@ -84,8 +84,8 @@ public class FileTreeIterator extends WorkingTreeIterator {
         *            the repository whose working tree will be scanned.
         */
        public FileTreeIterator(Repository repo) {
-               this(repo.getWorkTree(), repo.getFS(), WorkingTreeOptions
-                               .createConfigurationInstance(repo.getConfig()));
+               this(repo.getWorkTree(), repo.getFS(),
+                               repo.getConfig().get(WorkingTreeOptions.KEY));
                initRootIterator(repo);
        }
 
index c9257e39eb4ed065b80b330f3d2a744ae945f550..6365c119c250e360917d6237a10b49de9b8c010c 100644 (file)
@@ -540,13 +540,9 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
         * @param forceContentCheck
         *            True if the actual file content should be checked if
         *            modification time differs.
-        * @param checkFilemode
-        *            whether the executable-bit in the filemode should be checked
-        *            to detect modifications
         * @return true if content is most likely different.
         */
-       public boolean isModified(DirCacheEntry entry, boolean forceContentCheck,
-                       boolean checkFilemode) {
+       public boolean isModified(DirCacheEntry entry, boolean forceContentCheck) {
                if (entry.isAssumeValid())
                        return false;
 
@@ -563,7 +559,7 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
                // Ignore the executable file bits if checkFilemode tells me to do so.
                // Ignoring is done by setting the bits representing a EXECUTABLE_FILE
                // to '0' in modeDiff
-               if (!checkFilemode)
+               if (!state.options.isFileMode())
                        modeDiff &= ~FileMode.EXECUTABLE_FILE.getBits();
                if (modeDiff != 0)
                        // Report a modification if the modes still (after potentially
index 6929046c25b46bb9d0ad13a686d5a0a481404c35..630d00b376693ec7e0a2301a364a09aff5d61e66 100644 (file)
 package org.eclipse.jgit.treewalk;
 
 import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.CoreConfig;
+import org.eclipse.jgit.lib.Config.SectionParser;
 import org.eclipse.jgit.lib.CoreConfig.AutoCRLF;
 
-/**
- * Contains options used by the WorkingTreeIterator.
- */
+/** Options used by the {@link WorkingTreeIterator}. */
 public class WorkingTreeOptions {
+       /** Key for {@link Config#get(SectionParser)}. */
+       public static final Config.SectionParser<WorkingTreeOptions> KEY = new SectionParser<WorkingTreeOptions>() {
+               public WorkingTreeOptions parse(final Config cfg) {
+                       return new WorkingTreeOptions(cfg);
+               }
+       };
 
-       /**
-        * Creates default options which reflect the original configuration of Git
-        * on Unix systems.
-        *
-        * @return created working tree options
-        */
-       public static WorkingTreeOptions createDefaultInstance() {
-               return new WorkingTreeOptions(AutoCRLF.FALSE);
-       }
-
-       /**
-        * Creates options based on the specified repository configuration.
-        *
-        * @param config
-        *            repository configuration to create options for
-        *
-        * @return created working tree options
-        */
-       public static WorkingTreeOptions createConfigurationInstance(Config config) {
-               return new WorkingTreeOptions(config.get(CoreConfig.KEY).getAutoCRLF());
-       }
+       private final boolean fileMode;
 
-       /**
-        * Indicates whether EOLs of text files should be converted to '\n' before
-        * calculating the blob ID.
-        **/
        private final AutoCRLF autoCRLF;
 
-       /**
-        * Creates new options.
-        *
-        * @param autoCRLF
-        *            indicates whether EOLs of text files should be converted to
-        *            '\n' before calculating the blob ID.
-        */
-       public WorkingTreeOptions(AutoCRLF autoCRLF) {
-               this.autoCRLF = autoCRLF;
+       private WorkingTreeOptions(final Config rc) {
+               fileMode = rc.getBoolean("core", "filemode", true);
+               autoCRLF = rc.getEnum("core", null, "autocrlf", AutoCRLF.FALSE);
+       }
+
+       /** @return true if the execute bit on working files should be trusted. */
+       public boolean isFileMode() {
+               return fileMode;
        }
 
-       /**
-        * Indicates whether EOLs of text files should be converted to '\n' before
-        * calculating the blob ID.
-        *
-        * @return true if EOLs should be canonicalized.
-        */
+       /** @return how automatic CRLF conversion has been configured. */
        public AutoCRLF getAutoCRLF() {
                return autoCRLF;
        }